refactor: reorient CLAIM note consumption flow#2528
refactor: reorient CLAIM note consumption flow#2528partylikeits1983 wants to merge 52 commits intoagglayerfrom
CLAIM note consumption flow#2528Conversation
55ee9fb to
57cfbb5
Compare
Fumuran
left a comment
There was a problem hiding this comment.
Looks great, thank you! I left some nits and questions inline.
This is partial review, I only looked through the MASM code (but mostly in a way of code optimization, not the algorithm correctness).
Co-authored-by: Andrey Khmuro <[email protected]>
Co-authored-by: Andrey Khmuro <[email protected]>
Co-authored-by: Andrey Khmuro <[email protected]>
Co-authored-by: Andrey Khmuro <[email protected]>
Co-authored-by: Andrey Khmuro <[email protected]>
Co-authored-by: Andrey Khmuro <[email protected]>
Co-authored-by: Andrey Khmuro <[email protected]>
Co-authored-by: Andrey Khmuro <[email protected]>
Co-authored-by: Andrey Khmuro <[email protected]>
ee64a9b to
6a602df
Compare
Fumuran
left a comment
There was a problem hiding this comment.
Looks great, thank you! I left just one nit inline.
0d19a5e to
cfb6211
Compare
* 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 <[email protected]> * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <[email protected]> * fix: rename to output & fix copilot suggestion --------- Co-authored-by: Alexander John Lee <[email protected]> Co-authored-by: Copilot Autofix powered by AI <[email protected]> Co-authored-by: riemann <[email protected]>
mmagician
left a comment
There was a problem hiding this comment.
The merge changes look good now 👍🏼
I left a few more questions / open items to investigate, but other than those we should be ready to merge soon.
| const CLAIM_PROOF_DATA_WORD_LEN = 134 | ||
| const CLAIM_LEAF_DATA_WORD_LEN = 8 |
There was a problem hiding this comment.
yes it might need some more thinking, but at least we should not duplicate the constants that refer to the same underlying data, which these do
| # Compute note tag targeting the faucet account | ||
| loc_load.BUILD_MINT_FAUCET_PREFIX_LOC | ||
| # => [faucet_prefix, note_type, MINT_RECIPIENT, pad(16)] | ||
|
|
||
| exec.note_tag::create_account_target |
There was a problem hiding this comment.
I don't understand why this is the case, could you pinpoint why this is needed?
IIUC, this is a tag for the faucet, nothing to do with the P2ID that is created later?
The faucet is a network account, and the consumption of notes should not be determined by tag, but by note attachment.
Resolved.
I set the tag of the |
This PR reorients the flow in which the
CLAIMnote is consumed.This PR reorients the flow from:
CLAIM note is consumed by AggFaucet account, which does an FPI call to bridge to verify merkle proof, if valid creates P2ID note
to:
CLAIM note is consumed by AggBridge account which verifies the merkle proof, if valid creates a MINT note, which is then consumed by AggFaucet account in separate tx, which creates P2ID note.
This PR adds a mapping to the AggBridge which stores the
bytes20origin token address in the faucet registry ashash(bytes20)->AccountId.This is so that when creating the
MINTnote, the correct network note attachment target can be set. Storing the hash of the destination address as the key in a mapping, where the value is the aggfaucetAccountIdis essential, as previously we only stored registered AggFaucets in a mapping like this:AccountId->bool.Note: Both the
MINTnote &P2IDnote which created by the consumption of theMINTnote, both use thePROOF_DATA_KEY(hash of proof data) as their serial numbers.Resolves #2506