-
Notifications
You must be signed in to change notification settings - Fork 54
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
feat: adding note args and tests #442
Conversation
f83112e
to
c1dfa97
Compare
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.
Thank you! Looks good! I left a could of comment inline - the main one is about how we provide note args to the transaction executor.
Also, the MASM code will need to be updated a bit once this PR is rebased from the latest next
.
5b7cf65
to
759a0f8
Compare
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.
Thank you! Looks good. I left some comments inline. The main thing is that I think we can simplify the structure quite a bit by following a similar pattern as for note inputs. Specifically:
- In the prologue (i.e., in the
process_input_notes_data
procedure), we can get note args from the advice provider and put them next to other note data. - Then, right before the note is executed (e.g., as a part of
prepare_note
procedure), we would put note args onto the stack.
This way, when a note scripts starts executing, the state of the stack would be:
[SCRIPT_ROOT, NOTE_ARGS, ...]
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.
Thank you! Looks good! I left a some more comments inline. Most are pretty minor. The main one is about figuring out the right place to add note args into the process.
a98b425
to
d235dbc
Compare
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.
Thank you! I think this approach works better. I left some more comments inline.
pub struct TransactionArgs { | ||
tx_script: Option<TransactionScript>, | ||
note_args: Option<BTreeMap<NoteId, Word>>, | ||
} |
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 think this was supposed be be:
pub struct TransactionArgs {
tx_script: Option<TransactionScript>,
note_args: BTreeMap<NoteId, Word>,
}
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.
Not sure if I follow. NoteArgs
are optional to a transaction the same way as a TransactionScript
, right? So the canonical way to indicate this optionality would be
pub struct TransactionArgs {
tx_script: Option<TransactionScript>,
note_args: Option<BTreeMap<NoteId, Word>>,
}
Why would you put note_args: BTreeMap<NoteId, Word>
not as an option?
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.
An empty BTreeMap
is functionally equivalent to None
in this case but without the extra step we need to take to unwrap it.
eb51835
to
8bf5ad0
Compare
8bf5ad0
to
924bd42
Compare
Closes #338
Supersedes #374
Still unsure how we want a user to set note_args. Atm a user need to define note_args when creating the
InputNote