feat: handle standards scripts directly in TransactionExecutorHost w/o DataStore query#2417
feat: handle standards scripts directly in TransactionExecutorHost w/o DataStore query#2417
TransactionExecutorHost w/o DataStore query#2417Conversation
08790b2 to
0c6968b
Compare
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I left one optional comment inline.
| /// Handles a request for a [`NoteScript`] during transaction execution. | ||
| /// | ||
| /// The script is fetched from the data store and used to build a [`NoteRecipient`], which is | ||
| /// then used to create an [`OutputNoteBuilder`]. This function is only called for public notes | ||
| /// where the script is not already available in the advice provider. | ||
| /// Standard note scripts (P2ID, etc.) are resolved directly from [`StandardNote`], avoiding a | ||
| /// data store round-trip. Non-standard scripts are fetched from the [`DataStore`]. | ||
| /// | ||
| /// The resolved script is used to build a [`NoteRecipient`], which is then used to create | ||
| /// an [`OutputNoteBuilder`]. This function is only called for notes where the script is not | ||
| /// already available in the advice provider. | ||
| async fn on_note_script_requested( |
There was a problem hiding this comment.
Not from this PR, but it feels like the function name and the comments are a bit off here. IIUC, this is not so much about handling note script requests, but registering output notes - and specifically the branch when the script is not already in the advice provider. So, maybe a better name here would be something like register_output_note_with_missing_script()?
And the comments could be updated to reflect that we are registering an output note by first trying to get its script (either a standard script or one from the data store). Also, would be good to mention that this errors out if we cannot find a script for a public note (though, for private not it is fine).
There was a problem hiding this comment.
I would keep the naming pattern in line with the rest of the event handlers, i.e. on_<what happens>.
not so much about handling note script requests, but registering output notes
Building an output note is usually an end goal, but this callback is used as part of a smaller substep which is building the NoteRecipient. In theory, building a note recipient could happen on its own (though I'm not sure why anyone would do that), without the overarching intent to build an output note. And while I can't find a counter use case now, I still prefer to keep the method name tied closer to the existing functionality (caller builds a recipient and requests a note script) rather than to the goal (caller builds an output note).
…w/o `DataStore` query (0xMiden#2417) * feat: handle standards scripts directly w/o DataStore query * chore: no need to explicitly add standard scripts to tx context in tests * changelog * chore: add note to DataStore::get_note_script * chore: clarify docs and add panics
Resolve standard note scripts directly in `TransactionExecutorHost, avoiding a data store round-trip.
As described in 0xMiden/node#1616 (comment)
closes #2418