CLAIM note followup: helper functions & refactoring#2270
CLAIM note followup: helper functions & refactoring#2270partylikeits1983 merged 29 commits intoagglayerfrom
CLAIM note followup: helper functions & refactoring#2270Conversation
partylikeits1983
force-pushed
the
ajl-claim-note-follow-up
branch
3 times, most recently
from
b64be66 to
4b56f47
Compare
partylikeits1983
changed the base branch from
ajl-agglayer-asset-conversion
to
ajl-solidity-type-conversions
partylikeits1983
force-pushed
the
ajl-claim-note-follow-up
branch
from
4b56f47 to
e0c959b
Compare
partylikeits1983
added
no changelog
partylikeits1983
requested review from
bobbinth
and removed request for
bobbinth
bobbinth
left a comment
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you. Not a full review, but I left some comments inline about how the code can be improved further.
bobbinth
left a comment
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you. I left some more comments inline.
| @@ -128,17 +130,57 @@ proc batch_pipe_double_words | |||
| exec.mem::pipe_double_words_preimage_to_memory drop | |||
| end | |||
There was a problem hiding this comment.
@bobbinth this is where I validate that the advice map keys passed to the claim procedure do in fact hash to the values in the AdviceMap. Since this writes to global memory, then does an FPI call to the bridge, do we need to validate again that the destination_address is valid and the values in the AdviceMap were not tampered with?
| proc get_destination_account_id_data | ||
| mem_load.DESTINATION_ADDRESS_4 | ||
| mem_load.DESTINATION_ADDRESS_3 | ||
| mem_load.DESTINATION_ADDRESS_2 | ||
| mem_load.DESTINATION_ADDRESS_1 | ||
| mem_load.DESTINATION_ADDRESS_0 | ||
| # => [address[5]] | ||
|
|
||
| exec.eth_address::to_account_id | ||
| # => [prefix, suffix] | ||
| end |
There was a problem hiding this comment.
I forgot that the values were already in global memory, this procedure does not need to read from the AdviceMap, which simplifies things a lot.
mmagician
left a comment
mmagician
left a comment
There was a problem hiding this comment.
There are a few comments I left still worth addressing in this PR, but once these are done - ready to merge ✅
This looks much cleaner overall, thank you!
bobbinth
left a comment
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I left some comments inline - but I would address anything that is non-trivial in a follow-up PR.
| /// Global index (uint256 as 8 u32 values) | ||
| pub global_index: [u32; 8], |
There was a problem hiding this comment.
I would introduce a GlobalIndex type for this - but let's do it in a follow-up.
| // TODO: Make CLAIM note a Network Note once NoteAttachment PR lands | ||
| let tag = NoteTag::new(0); |
There was a problem hiding this comment.
Note attachment PR has already been merged, but let's make this change in the follow-up.
3 participants
This PR refactors
ClaimNoteParamsinto a nestedstructand ties in address conversion utility procedures into theCLAIMnote flow.Resolves item 1.1 & 2.8 of: #2237