-
Notifications
You must be signed in to change notification settings - Fork 105
Solidity address <> Miden AccountId helper functions
#2238
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
Conversation
address <> Miden AccountId helper functions
fe30e67 to
cf2dd5e
Compare
cf2dd5e to
46143c8
Compare
bobbinth
left a comment
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! Thank you. I left some more comments inline.
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.
I think there is a discrepancy between u32 and u64 usage.
IIUC, the changes in this PR propose a representation of the address such that the first element is zero, and the rest are u64s (w/o overflowing the max field size).
However, for hashing the elements with keccak, which is needed as part of #2220, we need all elements to fit inside u32s. And then we necessarily need all 5 u32s to be non-zeros to represent 160 bytes of the Ethereum address.
Let me know if this makes sense or whether I'm missing some context.
igamigo
left a comment
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.
Still getting familiar with some of this code but leaving a couple of comments
|
I clarified the byte/u32-limb/64-bit word representations in doc comments (and aligned Rust limb ordering with MASM), added explicit “no mod-reduction” checks when constructing felts from 64-bit words, and switched the I also simplified the |
mmagician
left a comment
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.
Unless I'm missing something, I have the same comment as before: this needs a rather fundamental rework still, because it is not possible to represent 20 non-zero bytes corresponding to an Ethereum address with 4 u32s (i.e. 128 bits), so the PR is conceptually incorrect.
crates/miden-testing/tests/agglayer/solidity_miden_address_conversion.rs
Show resolved
Hide resolved
mmagician
left a comment
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, with some small outstanding comments re: naming, efficiency and function structuring.
It would be good to still align on the naming & structure before merging, but otherwise ✅
crates/miden-testing/tests/agglayer/solidity_miden_address_conversion.rs
Show resolved
Hide resolved
crates/miden-testing/tests/agglayer/solidity_miden_address_conversion.rs
Show resolved
Hide resolved
bobbinth
left a comment
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! Thank you! I left some more comments inline. Once these are addresses, we should be good to go (but also would be good to get @mmagician's sign-off).
| #! The Ethereum address format is represented as 5 u32 limbs (20 bytes total) in *little-endian limb order*: | ||
| #! addr0 = bytes[16..19] (least-significant 4 bytes) | ||
| #! addr1 = bytes[12..15] | ||
| #! addr2 = bytes[ 8..11] | ||
| #! addr3 = bytes[ 4.. 7] | ||
| #! addr4 = bytes[ 0.. 3] (most-significant 4 bytes) |
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.
For some reason, I had previously thought that Ethereum addresses are natively in little-endian order - but it seems like they are in big-endian order. So, does this mean that to get the address in this form we need to reverse the bytes?
If so, it may be worth going back to the "native" order of Ethereum addresses - i.e., addr0 = bytes[0..3].
This PR implements conversion functions for Solidity
addresstype (bytes20) to MidenAccountId&Address.Concretely what functionality does this PR add?
This PR adds Rust conversion functions to convert
AccountIdinto solidityaddresstype and back. Additionally there is a masm procedure to convert theaddresstype represented as[5; Felt](part ofCLAIMnote inputs) intoAccountId. TLDR; part of bridging EVM<>MidenNOTE:
This PR does not assume it possible to represent any arbitrary Ethereum address as an AccountId.
This PR adds functions to be able to represent an
AccountIdas an Ethereum address (20 byte hex string) and adds conversion functions which can take this very specific ethereum address which represents anAccountIdand convert it back toAccountId.Why?
Because to bridge from EVM to Miden, a user will call the
bridgeAsset()function on an EVM chain. The destination address on Miden will be this ethereum address which represents anAccountIdon Miden.Builds on top of #2188
Added Rust helper functions:
account_id_to_ethereum_address_hex:AccountId-> Solidityaddresstypeethereum_address_hex_to_account_id: Solidityaddresstype toAccountIdethereum_address_to_felts: Solidityaddresstype toaddress[5](part ofCLAIMnote inputs)MASM helper procedures:
hashing a
address[5]type with keccak256 (might not be necessary, but this was a first test before implementing ImplementgetLeafValue()in MASM #2220)converting
address[5]type into anAccountIdtypeResolves: #2229