Compute claimed global index chain hash#2516
Conversation
There was a problem hiding this comment.
Looks great!
I just think we should reuse the pure function test helpers: test_utils::execute_program_with_default_host. I think using execute_program_with_default_host will clean up the test, and also make it possible to not have to have a "copy" of the compute_cgi_hash_chain procedure in the test.
Also lets update references to "chain hash" to "hash chain". Otherwise everything looks great!
| proc compute_cgi_hash_chain_copy | ||
| # load the global index onto the stack | ||
| push.GLOBAL_INDEX_PTR exec.utils::mem_load_double_word | ||
| # => [GLOBAL_INDEX[8], LEAF_VALUE[8]] | ||
|
|
||
| exec.keccak256::merge | ||
| # => [Keccak256(GLOBAL_INDEX, LEAF_VALUE), pad(8)] | ||
|
|
||
| # load the old CGI chain hash | ||
| push.OLD_CGI_CHAIN_HASH_PTR exec.utils::mem_load_double_word | ||
| # => [OLD_CGI_CHAIN_HASH[8], Keccak256(GLOBAL_INDEX, LEAF_VALUE), pad(8)] | ||
|
|
||
| # compute the new CGI chain hash | ||
| exec.keccak256::merge | ||
| # => [NEW_CGI_CHAIN_HASH[8], pad(8)] | ||
| end |
There was a problem hiding this comment.
I'd actually call the real procedure, not a copy of it. I think the asset conversion tests are a good example which show how to do this so that you don't have to run a full transaction.
| "# | ||
| ); | ||
|
|
||
| let tx_script = CodeBuilder::new() |
There was a problem hiding this comment.
I think it would be cleaner if we had the assert_eq! in Rust as opposed to running the test logic in masm. I think it reads easier if you call the masm procedure, get the returned values off the stack in Rust, then check the two Rust variable values are equal. As noted above, we have this testing functions available in the agglayer crate, check out the asset_conversion.rs file to see how they are used
There was a problem hiding this comment.
I recall that we had a discussion about whether we should use asserts in the Rust code vs MASM code, but IIRC we agreed to follow @PhilippGackstatter 's proposal to use asserts in the MASM code, but honestly I don't remember why. Probably to make the code self-contained and, which is more important, to check the error messages correctness in case the testing code uses some of the internal kernel/protocol procedures.
There was a problem hiding this comment.
I think we discussed it at some point, but don't recall the outcome. I think there are pros and cons. Multiple asserts in MASM read relatively nicely and can be done sequentially, as done in this test.
On the other hand, asserts in Rust generally result in a better error when they fail. The downside is if you need to assert the correctness of multiple values, you sometimes need to write extra MASM to arrange the output stack appropriately and then also extract multiple stack values in the right order from the ExecutionOutput, which is messy and incurs unnecessary mental load to review its correctness.
So, I'd use whichever approach results in a more readable and easily maintainble test and one that provides good errors when it fails.
Co-authored-by: Andrey Khmuro <andrey@polygon.technology>
Co-authored-by: Andrey Khmuro <andrey@polygon.technology>
Co-authored-by: Andrey Khmuro <andrey@polygon.technology>
Co-authored-by: Andrey Khmuro <andrey@polygon.technology>
Co-authored-by: Andrey Khmuro <andrey@polygon.technology>
Co-authored-by: Andrey Khmuro <andrey@polygon.technology>
Co-authored-by: Andrey Khmuro <andrey@polygon.technology>
Co-authored-by: Andrey Khmuro <andrey@polygon.technology>
Co-authored-by: Andrey Khmuro <andrey@polygon.technology>
Co-authored-by: Andrey Khmuro <andrey@polygon.technology>
* chore: pass faucet ID on stack to verify_claim_amount * chore: pass faucet ID on stack to build_mint_output_note * chore: completely remove CLAIM_FAUCET_ID_PREFIX_MEM_ADDR * chore: drop "y" in verify_u128_to_native_amount_conversion * chore: avoid uplicating y only to drop it later * chore: adjust comments; no longer reading faucetID from mem * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * fix: rename to output & fix copilot suggestion --------- Co-authored-by: Alexander John Lee <77119221+partylikeits1983@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Co-authored-by: riemann <aleqvids@gmail.com>
partylikeits1983
left a comment
There was a problem hiding this comment.
Looks good to me!
@mmagician is the plan to merge #2528 first, and then this PR?
crates/miden-agglayer/solidity-compat/test/ClaimAssetTestVectorsRealTx.t.sol
Show resolved
Hide resolved
crates/miden-agglayer/solidity-compat/test/ClaimAssetTestVectorsRealTx.t.sol
Outdated
Show resolved
Hide resolved
Merge 3 agglayer commits: claim flow reorientation (#2528), rollup claim processing (#2573), and global index chain hash (#2516). Conflict resolution: - bridge_config.masm: take agglayer (token registry additions) - faucet/mod.masm: take agglayer (claim removal, get_scale refactor), keep our get_metadata_hash procedure and storage slots - components/faucet.masm: export both get_metadata_hash and get_scale - faucet.rs: take agglayer ownable pattern (owner_config_slot), keep metadata hash slots and accessors Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This PR:
bridge_in.masmfile to add the CGI chain hash computation in theverify_leaf_bridgeprocedure. For now it is more like in a "template" state, since for now it is impossible to compute this value: to do so we need to read the previous CGI hash value, but currently it is not store anywhere (yet).verify_leaf_bridgeprocedure (or, to be precise, the value, computed using the same algorithm: I can't reuse thecompute_cgi_hash_chainprocedure itself since it is private and it uses some memory values which are not available during the test) is correct.We still need to store the computed value in memory, but this approach is blocked until we change the way SWAP note it consumed. Storing the CGI hash will be done in a separate PR.
Part of #2386