dfinity / motoko-base

The Motoko base library
Apache License 2.0
483 stars 99 forks source link

Add function that calculates ledger account from a principal #582

Closed kentosugama closed 1 year ago

kentosugama commented 1 year ago

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

kentosugama commented 1 year ago

I asked Timo if I could include his implementation here and he said yes.

It seems that converting Principal to Account is a basic enough function to have it in base. I hid the cryptographic functions as private functions though.

I will look into where the CRC32 is coming from.

ZenVoich commented 1 year ago

Won't it be confused with ICRC1 account?

Personally, I’m used to the fact that ledger account is accountId

kentosugama commented 1 year ago

It looks like the Ledger documentation specifies the prepending of the CRC32 hash

https://internetcomputer.org/docs/current/references/ledger#_accounts

chenyan-dfinity commented 1 year ago

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

kentosugama commented 1 year ago

I just tried making a call to the Ledger Canister with and without the CRC checksum. The call without the checksum failed. The code you linked to has logic for checking the checksum. I think internally, they strip off the CRC but the public interface requires it. (https://github.com/dfinity/ic/blob/a6d4159013996396c84ca33f54dd60f2c5e17ea0/rs/rosetta-api/icp_ledger/ledger.did#L13)

kentosugama commented 1 year ago

@ZenVoich that's a fair feedback and seems consistent with the documentation.

Maybe it should be called toAccountID(), toLedgerAccount(), or even toLedgerAccountId()

kentosugama commented 1 year ago

https://m7sm4-2iaaa-aaaab-qabra-cai.ic0.app/?tag=1188592470

This example seems to clearly show you need the CRC32.