Note lifecycle assertion macros#2833
Conversation
partylikeits1983
left a comment
partylikeits1983
left a comment
There was a problem hiding this comment.
Thank you for this! Looks good to me!
Process note for next time: please add a short PR description and link a tracking issue. For a 441-line addition introducing a new public module + five macros, that context speeds up review and leaves a design rationale to point back to later.
Approving once CI is green and the rename is in.
| use miden_protocol::account::AccountId; | ||
| use miden_protocol::account::auth::AuthScheme; | ||
| use miden_protocol::asset::{Asset, FungibleAsset}; | ||
| use miden_protocol::Word; |
There was a problem hiding this comment.
rustfmt is failing on this import block. The miden_protocol::* uses should be merged into a single use miden_protocol::{...} with Word reordered. make format from the repo root fixes it.
|
|
||
| /// Asserts the tx consumed the given note (checks `tx.input_notes()`). | ||
| /// | ||
| /// Tx-level counterpart to [`assert_note_consumed!`]. |
There was a problem hiding this comment.
cargo doc -D warnings is failing on three unresolved intra-doc links here and on the other macros (assert_note_created, assert_note_consumed_by, assert_note_consumed). Inside #[macro_export] macro_rules! blocks, rustdoc can't resolve bare macro names. Fully-qualify them ([`crate::assert_note_consumed!`]) or drop the link syntax.
| /// ); | ||
| /// ``` | ||
| #[macro_export] | ||
| macro_rules! assert_note_created { |
There was a problem hiding this comment.
Could you please rename the macros to make the naming less ambiguous? Right now assert_note_consumed_by! (tx-level) and assert_note_consumed! (chain-level) share a prefix and the _by does all the disambiguation work, which is too easy to misread. Prefix every macro with the scope:
assert_note_created!->assert_tx_created!assert_note_consumed_by!->assert_tx_consumed!assert_note_committed!->assert_chain_committed!assert_note_unspent!->assert_chain_unspent!assert_note_consumed!->assert_chain_consumed!
|
Thanks for the review @partylikeits1983, and apologies for skipping the PR description and issue link earlier - that was my mistake. I’ve now linked the tracking issue(#2805). I’ll also fix the remaining lint failures. For the chain-level note lifecycle checks, I’d lean toward following @PhilippGackstatter's suggestion and adding helpers on |
Address comments from the issuse 0xMiden#2805, drop the 4 macros in favor of plain helper methods. - `assert_note_committed!` -> `MockChain::is_note_committed` - `assert_note_unspent!` -> `MockChain::is_note_unspent` - `assert_note_consumed!` -> `MockChain::is_note_consumed` - `assert_note_consumed_by!` -> `ExecutedTransaction::consumes_note` Keep `assert_note_created!` since it does spec matching with optional fields.
marijamijailovic
force-pushed
the
pr/note-assertion-helpers
branch
from
8cb73d5 to
c1888d2
Compare
2 participants
This PRs closes #2805