-
Notifications
You must be signed in to change notification settings - Fork 15
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
feat: update SubAccount.formID to handle bigger numbers #526
feat: update SubAccount.formID to handle bigger numbers #526
Conversation
Dear @ENJATZ, In order to potentially merge your code in this open-source repository and therefore proceed with your contribution, we need to have your approval on DFINITY's CLA. If you decide to agree with it, please visit this issue and read the instructions there. Once you have signed it, re-trigger the workflow on this PR to see if your code can be merged. — The DFINITY Foundation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi,
Thanks for the PR.
Subaccounts seems indeed to be arbitrary 32-byte values source (I'm happy if one of my colleague confirm this) but I think NNS dapp limits the number of subaccounts to 256 source. That's why this limitation has probably been historically developed.
Therefore, the team would also need to replicate the logic in NNS dapp codebase before being able to merge this. I'm not mentioning this as a deal-breaker, just pointing out that it might require additional work on that side too.
My colleague @lmuntaner pointed out that NNS dapp frontend actually does not seem to use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks a lot!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks!
@ENJATZ: We just released your change in |
# Motivation We just released a new [version](https://github.com/dfinity/ic-js/releases/tag/2024.02.14-1600Z) of ic-js libs, this PR bump those dependencies to their next version. In comparison to the libs already included in main, extending ICP `Subaccount.fromId` to accept more than 256 (see [PR](dfinity/ic-js#526)) is the only notable change.
Motivation
I am having a scenario where I need to handle more than 256 subaccounts. This is my workaround for now:
where
numberToUint8Array32
is the updatedfromID
method.Changes
Updates the
SubAccount.fromID
method to handle up to nine quadrillion subaccounts (which is the max safe int).It creates a buffer of 32 bytes and uses
writeBigInt64BE
to write theid
using a 24 offest.Tests
Added one more test for
fromID
method underaccount_identifier.spec.ts
. All tests are passing.Todos