-
Notifications
You must be signed in to change notification settings - Fork 99
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
Add function that calculates ledger account from a principal #582
Conversation
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.
Is this the ledger account? I think we don't want to append CRC32 at the end.
Also, not sure if this belongs to base
. Timo's sha224 library is way better than what we include here. I happen to wrote a version based on his library: https://github.com/dfinity/canister-profiling/blob/main/crypto/motoko/src/sha.mo#L16
I asked Timo if I could include his implementation here and he said yes. It seems that converting I will look into where the CRC32 is coming from. |
Won't it be confused with ICRC1 account? Personally, I’m used to the fact that ledger account is |
It looks like the Ledger documentation specifies the prepending of the CRC32 hash https://internetcomputer.org/docs/current/references/ledger#_accounts |
Interesting. Better to clarify with the ledger team, because their code doesn't have CRC32 either: https://github.com/dfinity/ic/blob/master/rs/rosetta-api/icp_ledger/src/account_identifier.rs#L58 |
I just tried making a call to the Ledger Canister with and without the CRC checksum. The call without the checksum failed. |
@ZenVoich that's a fair feedback and seems consistent with the documentation. Maybe it should be called |
… into principal-to-account
https://m7sm4-2iaaa-aaaab-qabra-cai.ic0.app/?tag=1188592470 This example seems to clearly show you need the CRC32. |
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.
Looks good to me, though slightly overkill since principals are short.
However, since this a user request, let's get it in.
I guess another design would be to parameterize by the sha244 implementation, but perhaps that's too risky. Or add a separate Ledger library that contains this (and in future other Ledger related logic). I expect many programs will use Principal, but not need the Ledger functionality.
I'll let @chenyan-dfinity decide...
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.
All good. I just realized that in the Rust implementation, the to_vec()
method actually prepend the CRC32, so we are all good!
s0 := ivs[iv][0]; | ||
s1 := ivs[iv][1]; | ||
s2 := ivs[iv][2]; | ||
s3 := ivs[iv][3]; | ||
s4 := ivs[iv][4]; | ||
s5 := ivs[iv][5]; | ||
s6 := ivs[iv][6]; | ||
s7 := ivs[iv][7] |
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.
Just for completeness, commenting here on an already merged PR, and too minor to be worth opening this again. So just for the record.
Since we are only using Sha224 I would have hard-codes the values here and remove the ivs
array entirely since it also includes IVs for Sha256 which is confusing.
Feature requested by community: https://dx.internetcomputer.org/topic/180
Requires a SHA224 and CRC32 implementation. Implementation taken from Timo's implementation:
https://github.com/research-ag/sha2