feat: Utilize expected notes from `ExecutedTransaction` #329

igamigo · 2024-05-08T23:17:56Z

Closes #320

Now that ExecutedTransaction contains all output note details, we can use that for checking that the notes that we expected were all correctly generated by the transaction, and after that tracking them accordingly.
This PR also contains a small refactor related to the NoteScreener's use of the Store. It removes the lifetime in favor of Rc.

igamigo · 2024-05-09T00:11:45Z

@bobbinth don't mind the integration tests failing too much (I think there could be a bug related to tags on miden-base, still investigating), but just want to see if these are about what you had in mind

bobbinth

On a quick look this looks pretty much as I've envisioned it. I left a few comments regarding treatment of note headers. I don't think we run into these situations now - so, we could address these comments separately.

src/client/transactions/mod.rs

bobbinth · 2024-05-09T00:20:30Z

src/client/transactions/mod.rs

+        let mut relevant_notes = vec![];
+        let output_notes = executed_transaction.output_notes().iter().map(|n| match n {
+            OutputNote::Full(n) => n,
+            OutputNote::Header(_) => panic!("ExecutedTransaction should have all note details"),


This is not necessarily true - a transaction could produce a note for which we don't have full details. For example, this is possible when creating a P2ID note when the only thing we know is the recipient digest.

The implication is that while such a note an be inserted into the output_notes table, it cannot be inserted into the input_notes table.

I see. But in that case, how do we want it to behave in the context of this code? Neither NoteHeader nor Note allow for the scenario you mentioned. Or do we want to go back to adding rows to the output_notes table based on the expected_notes from the TransactionRequest?

Yeah - now that I think about it, the two variants of OutputNote should be Full and something like Reduced (but better name is needed). The Reduced variant would basically be NoteHeader + Assets. I'll create an issue for this in miden-base.

For this PR, what you have now is probably fine but I'd change panic!() into todo!() as this is not the final state of how it should work.

Created 0xPolygonMiden/miden-base#682.

src/client/transactions/mod.rs

bobbinth · 2024-05-09T00:41:06Z

src/client/transactions/mod.rs

+        let tx_note_auth_hashes: BTreeSet<Digest> = executed_transaction
+            .output_notes()
+            .iter()
+            .map(|n| match n {
+                OutputNote::Full(n) => n.authentication_hash(),
+                OutputNote::Header(_) => {
+                    panic!("ExecutedTransaction should always have every note's details")
+                },
+            })
+            .collect();


Similar comment to the ones above re full notes vs. headers.

Also, what is the reason for switching to authentication hashes here (from note IDs)?

Maybe this is related - but I actually think TransactionArgs::add_expected_output_note() should accept NoteDetails rather than Note objects as note metadata is not read from the advice provider but rather is built directly by note scripts.

Also, what is the reason for switching to authentication hashes here (from note IDs)?

It's a stronger check, would avoid problems like these unless I'm missing something

ah - makes sense!

…miden-client into igamigo-expected-notes

mFragaBA · 2024-05-10T15:20:35Z

src/client/transactions/mod.rs

+    pub fn new<S: Store>(
+        transaction: ExecutedTransaction,
+        note_screener: NoteScreener<S>,
+    ) -> Result<Self, ClientError> {
+        let mut relevant_notes = vec![];
+
+        for note in notes_from_output(transaction.output_notes()) {
+            let account_relevance = note_screener.check_relevance(note)?;
+
+            if !account_relevance.is_empty() {
+                relevant_notes.push(note.clone().into());
+            }
        }
-    }

-    pub fn executed_transaction(&self) -> &ExecutedTransaction {
-        &self.executed_transaction
+        let tx_result = Self { transaction, relevant_notes };
+
+        Ok(tx_result)
    }


should we also provide a way to create a TransactionResult without filtering by note relevance?

Might be a good idea, but since we provide and use this TransactionResult type, I think we should add it if we end up needing it.

mFragaBA · 2024-05-10T15:31:26Z

src/client/transactions/mod.rs

+        OutputNote::Full(n) => n,
+        // The following todo!() applies until we have a way to support flows where we have
+        // partial details of the note
+        OutputNote::Header(_) => todo!("For now, all details should be held in OutputNote::Fulls"),


In which case do we get an OutputNote::Header?

The OutputNote::Header will contain only partial details of a note. When the transaction has to be proven, it is shrinked. At the point where we are using it though, all details will be held in OutputNote::Full after having been picked up from the advice map (after the transcation execution)

don't think it's blocking yet, but for swap flows I think we create an OutputNote::Header when we consume the swap note

…miden-client into igamigo-expected-notes

src/client/transactions/mod.rs

mFragaBA · 2024-05-13T13:51:26Z

src/store/note_record/output_note_record.rs

+// CONVERSIONS
+// ================================================================================================
+
+// TODO: Improve conversions by implementing into_parts()


should we create an issue for this?

Yes, we probably should. Although I think it would belong in miden-base so that we can do into_parts() with domain types.

tomyrd

LGTM, left some small comments

tomyrd · 2024-05-13T13:55:00Z

src/client/transactions/mod.rs

 /// It contains an [ExecutedTransaction], a list of [Note] that describe the details of the notes
-/// created by the transaction execution, and a list of `usize` `relevant_notes` that contain the
-/// indices of `output_notes` that are relevant to the client
+/// created by the transaction execution, and a list of `relevant_notes` that contains the
+/// `output_notes` that the client has to store as input notes, based on the NoteScreener output
 pub struct TransactionResult {
-    executed_transaction: ExecutedTransaction,
-    output_notes: Vec<Note>,
-    relevant_notes: Option<BTreeMap<usize, Vec<(AccountId, NoteRelevance)>>>,
+    transaction: ExecutedTransaction,
+    relevant_notes: Vec<InputNoteRecord>,
 }


I don't think the new doc comment reflects what the struct stores. It talks about a list of [Note] and output_notes but that vec doesn't exist anymore in the struct.

Good catch, changed.

tomyrd · 2024-05-13T14:07:18Z

src/client/transactions/transaction_request.rs

+    pub const P2ID: &str = "0x0007b2229f7c8e3205a485a9879f1906798a2e27abd1706eaf58536e7cc3868b";
+    pub const P2IDR: &str = "0x418ae31e80b53ddc99179d3cacbc4140c7b36ab04ddb26908b3a6ed2e40061d5";
+    pub const SWAP: &str = "0x755c5dd2fd8d2aa8c2896e0e366a45bdc59c6d5ab543909db32ccfaf949c5826";


For another PR: Should we find a way to change/calculate these automatically? I guess thanks to the tests we would never forget to update them but it would be a good addition.

Yeah, we might have to. Originally this was a 'temporary' solution until the MAST refactors on the compiler. Additionally I thought that these scripts would not change too much, and even though the tests catch these errors there are a couple of things we can do to avoid having to recalculate them:

Provide a const way in miden-base to get these values (since the scripts live there).

Alternatively, we could provide a runtime method in miden-base to calculate these easily, because right now the only thing we can do is call create_p2id_note() and get the roots each time which is a bit hacky.

…miden-client into igamigo-expected-notes

mFragaBA

LGTM!

* feat: Integrate next changes * fix: Back to next * Fix conflicts * Use node next * fix: Tetss * refactor: reuse function to create mocked client * fix: update swap note script root * fix: Generalize Authenticator * Merge fixes and refactors * Lints * docs: update docs from changes to CLI (#336) * feat: Utilize expected notes from `ExecutedTransaction` (#329) * feat: Use expected notes from TransactionResult * fix: Re-add output note check * Remove unused code * Lints * Fix tests * fix: Tetss * Stash * fix: Address reviews * fix: Tests * fix: Lints * fix: Lints * Try fix test * fix: Merge conflicts * Fix tests * docs: Address reviews * Lints * fix: New script root * Use 0.3 * fix: Correct genesis toml file * fix: test fix con cargo make start-node * fix: temp fix to rename config paths * fix: Correct miden-node toml * fix: Ship our own node config files * Migrate to node 0.3 * feat: Use incoming MMR proof to add block authentications for notes created in past blocks (#337) * test: Integration tests run with main node * fix: Remove std::cmp::max * refactor: git-like usage commands (#338) * Move new account creation to command * Flatten account command * Refactor `default` option * Separate new account commands * Refactor `notes` command * Refactor import comand to infer type * Remove intermediate sucommand for export * Flatten tags command * Separate tx new commands * Fix documentation * Join p2id and p2idr to send command * Remove trait * Change comment suggestions * Clarify new account help comments * Ask for storage path if not specified * Fixed problem with listing notes with no filter * Added a message for default account updates * Change suggestions * Remove prompt for store path * Added all NotesFilter * Fix merge * feat: enable swap notes (#324) * feature: add swap tx request and store swap payback note * feature: insert input note record from payback note details * feature: show message to user on how to track payback notes * fix: integration test compilation * make swap tx send the offered asset * fix: temporary fix for partial output notes * test: add skeleton for integration test * fix: lint issues * test: add test for onchain swap flow * test: add test for offchain swap flow * test: fix note minting for swap tests * test: fix compilation errors * fix: use all committed note ids to check transactions to commit * fix: fix tests checks * address review comments * docs: update CHANGELOG * docs: update CLI reference * address remaining comment * fix: clippy issues * fix: remove wrong check * refactor: store partial notes in relevant notes of transaction result * rephrase doc comment Co-authored-by: igamigo <[email protected]> * address review comments * fix: fix offchain swap test --------- Co-authored-by: igamigo <[email protected]> * docs: fix doc comments for swap clap command * feat: update cli-reference with new command structure (#342) * Update cli-reference with new command structure * Fix merge * Fix merge * Update cli-reference.md --------- Co-authored-by: igamigo <[email protected]> * Update README.md * CLI config.md --------- Co-authored-by: Martin Fraga <[email protected]> Co-authored-by: Martin Fraga <[email protected]> Co-authored-by: Tomas Rodriguez Dala <[email protected]>

igamigo added 3 commits May 8, 2024 20:00

feat: Use expected notes from TransactionResult

4bef43b

fix: Re-add output note check

04e6d33

Remove unused code

86403c0

igamigo requested a review from bobbinth May 8, 2024 23:17

igamigo added 2 commits May 8, 2024 20:22

Lints

2dbb681

Fix tests

261e420

bobbinth reviewed May 9, 2024

View reviewed changes

igamigo added 8 commits May 8, 2024 23:11

fix: Tetss

add0a13

Stash

0c85c91

fix: Address reviews

b36d05a

fix: Tests

b0103b0

Merge branch 'igamigo-next-0.3' of https://github.com/0xPolygonMiden/…

e0b0152

…miden-client into igamigo-expected-notes

fix: Lints

a90dda9

fix: Lints

a2cdb30

Try fix test

e2c0385

mFragaBA reviewed May 10, 2024

View reviewed changes

igamigo added 5 commits May 10, 2024 15:24

Merge branch 'igamigo-next-0.3' into igamigo-expected-notes

479303c

fix: Merge conflicts

b9e9b39

Merge branch 'igamigo-next-0.3' into igamigo-expected-notes

e32ca06

Merge branch 'igamigo-next-0.3' of https://github.com/0xPolygonMiden/…

adf01ea

…miden-client into igamigo-expected-notes

Fix tests

f2933fd

mFragaBA reviewed May 13, 2024

View reviewed changes

src/client/transactions/mod.rs Show resolved Hide resolved

mFragaBA reviewed May 13, 2024

View reviewed changes

tomyrd approved these changes May 13, 2024

View reviewed changes

igamigo added 3 commits May 13, 2024 14:09

docs: Address reviews

9b0c0b7

Lints

bd59192

Merge branch 'igamigo-next-0.3' of https://github.com/0xPolygonMiden/…

57d3c93

…miden-client into igamigo-expected-notes

mFragaBA approved these changes May 13, 2024

View reviewed changes

fix: New script root

1e39716

igamigo merged commit d7f363f into igamigo-next-0.3 May 13, 2024
6 checks passed

igamigo deleted the igamigo-expected-notes branch May 13, 2024 19:16

mFragaBA mentioned this pull request May 15, 2024

Use BTreeSet to check for duplicate note tags #320

Closed

igamigo mentioned this pull request May 15, 2024

feat: Integrate newer changes #321

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Utilize expected notes from `ExecutedTransaction` #329

feat: Utilize expected notes from `ExecutedTransaction` #329

igamigo commented May 8, 2024 •

edited

Loading

igamigo commented May 9, 2024

bobbinth left a comment

bobbinth May 9, 2024

igamigo May 9, 2024

bobbinth May 9, 2024

bobbinth May 9, 2024

bobbinth May 9, 2024

bobbinth May 9, 2024

igamigo May 9, 2024

bobbinth May 9, 2024

mFragaBA May 10, 2024

igamigo May 10, 2024

mFragaBA May 10, 2024

igamigo May 10, 2024

mFragaBA May 13, 2024

mFragaBA May 13, 2024

igamigo May 13, 2024

tomyrd left a comment

tomyrd May 13, 2024

igamigo May 13, 2024

tomyrd May 13, 2024

igamigo May 13, 2024

mFragaBA left a comment

feat: Utilize expected notes from ExecutedTransaction #329

feat: Utilize expected notes from ExecutedTransaction #329

Conversation

igamigo commented May 8, 2024 • edited Loading

igamigo commented May 9, 2024

bobbinth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomyrd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mFragaBA left a comment

Choose a reason for hiding this comment

feat: Utilize expected notes from `ExecutedTransaction` #329

feat: Utilize expected notes from `ExecutedTransaction` #329

igamigo commented May 8, 2024 •

edited

Loading