feat(AggLayer claim e2e #2): byte packing from felts#2401
feat(AggLayer claim e2e #2): byte packing from felts#2401mmagician merged 18 commits intoagglayer-newfrom
Conversation
mmagician
force-pushed
the
mmagician-get-leaf-test-vec
branch
7 times, most recently
from
8094a1f to
0b1c93f
Compare
mmagician
added
the
agglayer
mmagician
changed the title
mmagician
force-pushed
the
mmagician-get-leaf-test-vec
branch
from
18439d5 to
be2e67a
Compare
mmagician
changed the base branch from
agglayer-new
to
mmagician-padding-end-of-claim
mmagician
added
the
no changelog
mmagician
added
the
pr-from-maintainers
partylikeits1983
left a comment
partylikeits1983
left a comment
There was a problem hiding this comment.
Looks great! I think I've wrapped my head around the new byte packing format you've added.
The minor concern I have is the removal of the methods from EthAmount type, as I liked those helper methods a lot and used them extensively in my u256 to Felt scale verification PR :)
Other than that this PR LGTM! If you do think it is essential to remove those methods from EthAmount type, lets merge this, will refactor the scaling PR to be more inline with the style of byte ordering in this PR.
| #! The Keccak precompile expects u32 values packed in little-endian byte order. | ||
| #! For each packed element, we drop the leading 3 bytes and rebuild the u32 so that | ||
| #! bytes [b0, b1, b2, b3] map to u32::from_le_bytes([b0, b1, b2, b3]). | ||
| #! With little-endian input limbs, the first byte comes from the MSB of `curr` and | ||
| #! the next three bytes come from the LSBs of `next`: | ||
| #! packed = ((curr >> 24) & 0xFF) | ||
| #! | (next & 0xFF) << 8 | ||
| #! | ((next >> 8) & 0xFF) << 16 | ||
| #! | ((next >> 16) & 0xFF) << 24 | ||
| #! | ||
| #! Inputs: [] | ||
| #! Outputs: [] | ||
| #! | ||
| #! Invocation: exec |
There was a problem hiding this comment.
Nit: I think it would make a bit more sense if you outlined the state of memory before and after calling this procedure. Although it looks like the procedure does not take any inputs / outputs on the stack, it does but via memory. It would also make it more concrete what the before / after of memory looks like when calling this proc.
I think it is fine to use global memory like this because this procedure will only ever be called in the context of a network transaction, so it won't ever be used in a "user" context.
I think on all of my PRs where I had procedures like this which took global memory as an input, I think @bobbinth & @PhilippGackstatter thought it would be better to pass the data via the AdviceStack, that being said, I think its justified to use global memory like this because we are preparing the data before calling the Keccak precompile which requires the data to be in global memory.
There was a problem hiding this comment.
yes good call, will add a paragraph on the layout
There was a problem hiding this comment.
actually I cherry-picked chore: refactor into get_and compute_ leaf_value, so that this procedure actually becomes parametrized, and no longer implicitly relies on hardcoded memory poiners.
| #! addr2 = bytes[ 8..11] | ||
| #! addr3 = bytes[ 4.. 7] | ||
| #! addr4 = bytes[ 0.. 3] (most-significant 4 bytes) | ||
| #! The Ethereum address format is represented as 5 u32 limbs (20 bytes total) in *big-endian limb order* |
There was a problem hiding this comment.
are we going to have to change to make this little endian anyways with the new VM version? 🥲
| remappings = ["@agglayer/=lib/agglayer-contracts/contracts/"] | ||
| remappings = [ | ||
| "@agglayer/=lib/agglayer-contracts/contracts/", | ||
| "@openzeppelin/contracts-upgradeable4/=lib/openzeppelin-contracts-upgradeable/contracts/", |
There was a problem hiding this comment.
Why do we need the OZ lib & submodule?
| impl EthAmount { | ||
| /// Creates a new [`EthAmount`] from an array of 8 u32 values. | ||
| /// | ||
| /// The values are stored in little-endian order where `values[0]` contains | ||
| /// the least significant 32 bits. | ||
| pub const fn new(values: [u32; 8]) -> Self { | ||
| Self(values) | ||
| } | ||
|
|
||
| /// Creates an [`EthAmount`] from a single u64 value. | ||
| /// | ||
| /// This is useful for smaller amounts that fit in a u64. The value is | ||
| /// stored in the first two u32 slots with the remaining slots set to zero. | ||
| pub const fn from_u64(value: u64) -> Self { | ||
| let low = value as u32; | ||
| let high = (value >> 32) as u32; | ||
| Self([low, high, 0, 0, 0, 0, 0, 0]) | ||
| } | ||
|
|
||
| /// Creates an [`EthAmount`] from a single u32 value. | ||
| /// | ||
| /// This is useful for smaller amounts that fit in a u32. The value is | ||
| /// stored in the first u32 slot with the remaining slots set to zero. | ||
| pub const fn from_u32(value: u32) -> Self { | ||
| Self([value, 0, 0, 0, 0, 0, 0, 0]) | ||
| } | ||
|
|
||
| /// Returns the raw array of 8 u32 values. | ||
| pub const fn as_array(&self) -> &[u32; 8] { | ||
| &self.0 | ||
| } | ||
|
|
||
| /// Converts the amount into an array of 8 u32 values. | ||
| pub const fn into_array(self) -> [u32; 8] { | ||
| self.0 | ||
| } | ||
|
|
||
| /// Returns true if the amount is zero. | ||
| pub fn is_zero(&self) -> bool { | ||
| self.0.iter().all(|&x| x == 0) | ||
| } | ||
|
|
||
| /// Attempts to convert the amount to a u64. | ||
| /// | ||
| /// # Errors | ||
| /// Returns [`EthAmountError::Overflow`] if the amount doesn't fit in a u64 | ||
| /// (i.e., if any of the upper 6 u32 values are non-zero). | ||
| pub fn try_to_u64(&self) -> Result<u64, EthAmountError> { | ||
| if self.0[2..].iter().any(|&x| x != 0) { | ||
| Err(EthAmountError::Overflow) | ||
| } else { | ||
| Ok((self.0[1] as u64) << 32 | self.0[0] as u64) | ||
| } | ||
| } | ||
|
|
||
| /// Attempts to convert the amount to a u32. | ||
| /// | ||
| /// # Errors | ||
| /// Returns [`EthAmountError::Overflow`] if the amount doesn't fit in a u32 | ||
| /// (i.e., if any of the upper 7 u32 values are non-zero). | ||
| pub fn try_to_u32(&self) -> Result<u32, EthAmountError> { | ||
| if self.0[1..].iter().any(|&x| x != 0) { | ||
| Err(EthAmountError::Overflow) | ||
| } else { | ||
| Ok(self.0[0]) | ||
| } | ||
| /// Creates an [`EthAmount`] from a 32-byte array. | ||
| pub fn new(bytes: [u8; 32]) -> Self { | ||
| Self(bytes) | ||
| } | ||
|
|
||
| /// Converts the amount to a vector of field elements for note storage. | ||
| /// | ||
| /// Each u32 value in the amount array is converted to a [`Felt`]. | ||
| pub fn to_elements(&self) -> [Felt; 8] { | ||
| let mut result = [Felt::ZERO; 8]; | ||
| for (i, &value) in self.0.iter().enumerate() { | ||
| result[i] = Felt::from(value); | ||
| } | ||
| result | ||
| pub fn to_elements(&self) -> Vec<Felt> { | ||
| bytes_to_packed_u32_felts(&self.0) |
There was a problem hiding this comment.
Its a bit unfortunate that so many methods from the EthAmount type are being removed, these methods are extremely useful in #2331
There was a problem hiding this comment.
I think it's best to keep the public API surface to a minimum.
In practice, the EthAmount should only be instantiated from bytes (and/or from decimals string?)
If really required, we could have other constructors under a cfg(test), but I think we should try to use the intended API and even for tests construct EthAmounts from decimal strings.
| let mut bytes = [0u8; 32]; | ||
| for (i, limb) in limbs.iter().enumerate() { | ||
| /// Converts Felt u32 limbs to bytes using little-endian byte order. | ||
| /// TODO remove once we move to v0.21.0 which has `packed_u32_elements_to_bytes` |
There was a problem hiding this comment.
With VM version v0.21.0 won't we need to change the stack ordering from BE to LE as well?
mmagician
force-pushed
the
mmagician-get-leaf-test-vec
branch
from
97abbe1 to
b658ab6
Compare
chore: remove u32_words_to_solidity_bytes32_hex
feat: change Address ordering to BE limbs
Co-authored-by: Alexander John Lee <77119221+partylikeits1983@users.noreply.github.com>
mmagician
force-pushed
the
mmagician-get-leaf-test-vec
branch
from
b658ab6 to
ea8d6d7
Compare
|
I rebased onto |
2 participants
Tests for
getLeafValueusing real onchain dataDepositContractV2.getLeafValueon real onchain data, and use this expected value in our testsByte packing from
FeltsIt turns out that while the test that directly loaded packed bytes for
get_leaf_valuewas ok, we were missing the step of packing these values in MASM before hashing.What we had:
And for
getLeafValueto match, we need to prepare the felts as such:So effectively we needed to shift the byte-representation of felts left by 3 bytes.
This also means I had to move padding to the end of LEAF_DATA.
MetadataHashI also ended up creating a
MetadataHashstruct as a helperEthAddressFormatusing BE as the underlying representationIt's really useful if we can do the following:
{ "amount": "0x0000000000000000000000000000000000000000000000001bc16d674ec80000", "destination_address": "0xD9b20Fe633b609B01081aD0428e81f8Dd604F5C5", "destination_network": 7, "leaf_type": 0, "leaf_value": "0xb67e42971034605367b7e92d1ad1d4648c3ffe0bea9b08115cd9aa2e616b2f88", "metadata_hash": "0x6c7a91a5fb41dee8f0bc1c86b5587334583186f14acfa253e2f7c2833d1d6fdf", "origin_network": 0, "origin_token_address": "0xD9343a049D5DBd89CD19DC6BcA8c48fB3a0a42a7" }directly into a test vector.
LeafDatalike so:leaf_data.to_elements()to prepare the note inputs & everything else works out of the box when we pack the bytes and hash them.To achieve this,
EthAddressFormat'sto_elements()method must return felts with LE-byte order.But to build the account ID from these bytes, we need to swap bytes LE->BE within each byte, because that's what Miden's
AccountIdexpects.For this, I added
swap_u32_bytes, which is not very neat, but it does allow the user to basically take a MidenAccountIdin hex, append some zeros, and it corresponds 1-1 to what will appear as part ofclaimAssettx data on Ethereum.For example:
0x00000000b0E79c68cafC54802726C6F102Cca300AccountId(in hex): 0xb0E79c68cafC54802726C6F102Cca3` (the trailing 8 bits of our ID are zero, and are cut off from the hex repr)