Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ts-sdk: perform proper serialization of amount to u256 #325

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dhruvja
Copy link
Collaborator

@dhruvja dhruvja commented May 17, 2024

Use LE format to convert the bigint to 32 byte array instead of 8 byte done previously.

Comment on lines +18 to +25
const buffer = Buffer.alloc(32);
let numberHex = num.toString(16);
if (numberHex.length % 2 !== 0) {
numberHex = "0" + numberHex;
}
const numberBytes = Buffer.from(numberHex, "hex");
numberBytes.reverse().copy(buffer, 0);
return new Uint8Array(buffer);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps worth checking range? Also perhaps this is more straightforward:

Suggested change
const buffer = Buffer.alloc(32);
let numberHex = num.toString(16);
if (numberHex.length % 2 !== 0) {
numberHex = "0" + numberHex;
}
const numberBytes = Buffer.from(numberHex, "hex");
numberBytes.reverse().copy(buffer, 0);
return new Uint8Array(buffer);
const n = BigInt.asUintN(256, num);
if (num < 0 || n < num) {
throw new RangeError();
}
const numHex = '00'.repeat(32) + n.toString(16);
const numBuf = Buffer.from(numHex.substring(numHex.length - 64), 'hex');
return new Uint8Array(numBuf.reverse());

not tested.

Copy link
Collaborator Author

@dhruvja dhruvja May 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ya i think the code i came up with is prolly wrong. Since the U256 is [u64; 4] and convert to le, bytes are assigned from the back of the 32 byte array which is prolly not right. It should be converted to LE and then be split into 4 arrays and appended reversely.
Something like this.

export function numberTo32ByteBuffer(num: bigint): Uint8Array {
  // Create a buffer of 32 bytes initialized with zeros
  let buffer = Buffer.alloc(32);

  // Convert the BigInt to a hexadecimal string
  let numberHex = num.toString(16);
  if (numberHex.length % 2 !== 0) {
    numberHex = "0" + numberHex; // Ensure the hex string has an even length
  }

  // Create a buffer from the hexadecimal string
  let numberBytes = Buffer.from(numberHex, "hex");

  // Copy the number bytes to the end of the 32-byte buffer
  numberBytes.copy(buffer, 32 - numberBytes.length);
  let uintBuffer = new Uint8Array(buffer);
  // split above array into 4 chunks of 8 bytes each
  let uintBufferChunks = [];
  for (let i = 0; i < uintBuffer.length; i += 8) {
    uintBufferChunks.push(uintBuffer.slice(i, i + 8));
  }
  let final_uintBuffer: Array<number> = [];
  final_uintBuffer = final_uintBuffer.concat(...uintBufferChunks[3]);
  final_uintBuffer = final_uintBuffer.concat(...uintBufferChunks[2]);
  final_uintBuffer = final_uintBuffer.concat(...uintBufferChunks[1]);
  final_uintBuffer = final_uintBuffer.concat(...uintBufferChunks[0]);
  return new Uint8Array(final_uintBuffer);
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still unresolved, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well actually no, the above seems to work. The FE is using that only.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But this will break if number is over u64::MAX, no? Because of the weird way U256 is serialised in IBC (each u64 is serialised using little endian but the four u64 are serialised in big endian order IIRC).

@dhruvja dhruvja requested a review from mina86 August 22, 2024 07:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants