-
Notifications
You must be signed in to change notification settings - Fork 234
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
refactor: slot part of note hiding point preimage #7767
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -469,35 +469,41 @@ contract TokenWithRefunds { | |
// to the user in the `complete_refund(...)` function. | ||
storage.balances.sub(user, U128::from_integer(funded_amount)).emit(encode_and_encrypt_note_with_keys(&mut context, user_ovpk, user_ivpk, user)); | ||
|
||
// 4. We create the partial notes for the fee payer and the user. | ||
// 4. Now we "manually" compute the slots (by setting the slots we insert the notes to the balances map under | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think computing the slot here manually is pretty ugly. If we decide to make partial notes a first class citizen I think it would make sense to modify the state vars such that they return the note hiding point instead of immediately emitting the note hashes. Or maybe just having separate for 2 flows on the state vars (one returning note hiding point, another directly emitting). What does the reviewer think? |
||
// the correct keys) | ||
let fee_payer_balances_slot = derive_storage_slot_in_map(TokenWithRefunds::storage().balances.slot, fee_payer); | ||
let user_balances_slot = derive_storage_slot_in_map(TokenWithRefunds::storage().balances.slot, user); | ||
|
||
// 5. We create the partial notes for the fee payer and the user. | ||
// --> Called "partial" because they don't have the amount set yet (that will be done in `complete_refund(...)`). | ||
let fee_payer_partial_note = TokenNote { | ||
header: NoteHeader::empty(), | ||
header: NoteHeader { | ||
contract_address: AztecAddress::zero(), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am just setting the address to zero since it's unused. @nventuro would make the decision whether to nuke it as it's looking pretty weird here. |
||
nonce: 0, | ||
storage_slot: fee_payer_balances_slot, | ||
note_hash_counter: 0 | ||
}, | ||
amount: U128::zero(), | ||
npk_m_hash: fee_payer_npk_m_hash, | ||
randomness: fee_payer_randomness | ||
}; | ||
let user_partial_note = TokenNote { | ||
header: NoteHeader::empty(), | ||
header: NoteHeader { | ||
contract_address: AztecAddress::zero(), | ||
nonce: 0, | ||
storage_slot: user_balances_slot, | ||
note_hash_counter: 0 | ||
}, | ||
amount: U128::zero(), | ||
npk_m_hash: user_npk_m_hash, | ||
randomness: user_randomness | ||
}; | ||
|
||
// 5. Now we get the note hiding points. | ||
// 6. Now we get the note hiding points. | ||
let mut fee_payer_point = fee_payer_partial_note.to_note_hiding_point(); | ||
let mut user_point = user_partial_note.to_note_hiding_point(); | ||
|
||
// 6. Now we "manually" compute the slot points and add them to hiding points. | ||
let fee_payer_balances_slot = derive_storage_slot_in_map(TokenWithRefunds::storage().balances.slot, fee_payer); | ||
let user_balances_slot = derive_storage_slot_in_map(TokenWithRefunds::storage().balances.slot, user); | ||
|
||
// 7. We add the slot to the points --> this way we insert the notes into the balances Map under the respective key. | ||
// TODO(#7753): Consider making slots part of the initital note hiding point creation. | ||
fee_payer_point.add_slot(fee_payer_balances_slot); | ||
user_point.add_slot(user_balances_slot); | ||
|
||
// 8. Set the public teardown function to `complete_refund(...)`. Public teardown is the only time when a public | ||
// 7. Set the public teardown function to `complete_refund(...)`. Public teardown is the only time when a public | ||
// function has access to the final transaction fee, which is needed to compute the actual refund amount. | ||
context.set_public_teardown_function( | ||
context.this_address(), | ||
|
@@ -513,14 +519,14 @@ contract TokenWithRefunds { | |
#[aztec(public)] | ||
#[aztec(internal)] | ||
fn complete_refund( | ||
// TODO: the following makes macros crash --> try getting it work once we migrate to metaprogramming | ||
// TODO(#7771): the following makes macros crash --> try getting it work once we migrate to metaprogramming | ||
// mut fee_payer_point: TokenNoteHidingPoint, | ||
// mut user_point: TokenNoteHidingPoint, | ||
fee_payer_point_immutable: TokenNoteHidingPoint, | ||
user_point_immutable: TokenNoteHidingPoint, | ||
funded_amount: Field | ||
) { | ||
// TODO: nuke the following 2 lines once we have mutable args | ||
// TODO(#7771): nuke the following 2 lines once we have mutable args | ||
let mut fee_payer_point = fee_payer_point_immutable; | ||
let mut user_point = user_point_immutable; | ||
|
||
|
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 placed the assert here to sanity check that everywhere in the codebase the slot is assigned before we try to compute note hiding point. In plenty of the state vars we have the slot check inserted as well.
I think it would make sense to keep the slot check in all the notes and nuke it from the state vars. Does the reviewer agree?
@LHerskind I remember you once explained to me why the 0 slot was not working. I think it had something to do with pedersen but I can't exactly remember now. Do you still remember what was the cause? Maybe we can nuke the checks altogether now that pedersen is not used to compute note hashes.