chore(XC-48): extract cketh minter minicbor encoder and decoders into a separate library #2769
Conversation
rs/cross-chain/cbor/BUILD.bazel
Outdated
| # Keep sorted. | ||
| "//rs/phantom_newtype", | ||
| "@crate_index//:candid", | ||
| "@crate_index//:ethnum", |
There was a problem hiding this comment.
Is-it acceptable for such a library to have some Etherereum-dependency? This library is fairly self-contained, just adding a new integer type u256 so that could be acceptable. I think the ledgers already use that library for the u256 ledger variants. Alternatively, one could hide it behind a feature flag, but it seems overkill right now; or use a native type equivalent type such as a 32-byte array. I'm also ok leaving as is for now. WDYT?
There was a problem hiding this comment.
IIUC this code is already used in prod? In that case, maybe such changes could be done in a follow up PR?
There was a problem hiding this comment.
IIUC this code is already used in prod?
I'm not sure what you mean, since this is a new library. My point there is that having an Ethereum-dependency is a bit weird, because that it's a general purpose library that will be used for non-Ethereum stuff, like the ledgers.
ethnum is already used for ic-icrc1-tokens-u256 here but not for ic-icrc1-tokens-u64. So if you use that library for both ledgers variants, you will have a dependency on ethnum for both variants, which seems undesirable (cc @mbjorkqvist). If that's the case, do you prefer to handle this in a separate PR (I'm ok either way)?
There was a problem hiding this comment.
If /rs/ethereum/cketh/minter/src/cbor is already used in prod, it means that the encoding is fixed and any changes we make have to be backwards compatible and can be done at any point in the future. If this code is not used in prod, we can still decide how to encode u256...
There was a problem hiding this comment.
I agree that having ethereum dependency in generic u256 type is undesirable. I was just wondering if we can postpone making this change.
There was a problem hiding this comment.
/rs/ethereum/cketh/minter/src/cbor is already used in prod
It's used in prod.
I would not change the byte-encoding at all. Note that there is a bijection between a ethnum::u256 and a Rust primitive type, e.g., [u8; 32] so that it would for example be possible to have the library use the Rust primitive type, while the minter maps the Rust primitive type to aethnum::u256. That or have a crate feature flag.
packages/icrc-cbor/CHANGELOG.md
Outdated
There was a problem hiding this comment.
Here for a lack of a better place: you also need to add an entry in .github/CODEOWNERS
No description provided.