refactor(transaction): Validations for transaction requests #447

SantiagoPittella · 2024-07-24T16:41:24Z

Closes #444

crates/rust-client/src/transactions/mod.rs

tests/integration/common.rs

crates/rust-client/src/transactions/mod.rs

tests/integration/common.rs

igamigo · 2024-07-30T21:45:03Z

BTW maybe we should also add a validation for duplicate expected output notes/own notes as well.

tests/integration/common.rs

igamigo

LGTM. I left a couple non-blocking comments, except for one related to making sure we fail if the asset vault does not contain an asset that the notes require.

crates/rust-client/src/transactions/mod.rs

bobbinth

Thank you! Overall, this looks good - but I just realized that there is one thing we are missing: assets may be deposited into the account by input notes and so just because the account doesn't have all outgoing assets, doesn't mean the transaction will fail.

So, I think we could do the following:

Sum up all the incoming assets.
Sum up all the outgoing assets.
Make sure account_assets + incoming_assets >= outgoing_assets.

Also, I think making similar checks for faucets is a bit more involved - so, I'd remove them from this PR and create a separate issue for them.

bobbinth · 2024-08-01T23:45:55Z

crates/rust-client/src/transactions/mod.rs

+    fn calculate_outgoing_assets(
+        &self,
+        transaction_request: &TransactionRequest,
+    ) -> (BTreeMap<AccountId, u64>, BTreeSet<NonFungibleAsset>) {


nit: calculate_ prefix sounds a bit odd here. I'd probably use build_ or get_.

Also, I don't think we are using anything from self here. So, this can probably be a pure function and we could move it to the "Helper functions" section at the end of the file.

Another thought is that this could be a method on TransactionRequest.

bobbinth · 2024-08-01T23:46:38Z

crates/rust-client/src/transactions/mod.rs

+        let (fungible_balance_map, non_fungible_set) =
+            self.calculate_outgoing_assets(transaction_request);
+
+        // Get client assets


nit: should this say "Get account assets"?

bobbinth · 2024-08-01T23:53:29Z

crates/rust-client/src/transactions/mod.rs

+        // Faucet account can only mint one asset (itself)
+        if own_notes_assets.len() > 1 {
+            return Err(ClientError::TransactionRequestError(
+                TransactionRequestError::InvalidFaucetMint(
+                    "Faucet can only mint one asset".to_string(),
+                ),
+            ));
+        }


This is only true for fungible assets. We don't currently have non-fungible faucet fully working, so this may be OK for now - but also maybe we should make the check more sophisticated here.

bobbinth · 2024-08-01T23:55:14Z

crates/rust-client/src/transactions/mod.rs

+        let own_notes_assets = match transaction_request.script_template() {
+            Some(TransactionScriptTemplate::SendNotes(notes)) => notes
+                .iter()
+                .map(|note| note.assets())
+                .flat_map(|assets| assets.iter())
+                .collect::<Vec<_>>(),
+            _ => Default::default(),
+        };


I'm not sure this is sufficient to compute own_notes_assets. In theory, it should be possible for a note script to call the distribute procedure as well.

bobbinth · 2024-08-02T22:38:28Z

crates/rust-client/src/transactions/request.rs

+    #[maybe_async]
+    pub fn get_incoming_assets(
+        &self,
+        client: &Client<
+            impl NodeRpcClient,
+            impl FeltRng,
+            impl Store,
+            impl TransactionAuthenticator,
+        >,
+    ) -> Result<(BTreeMap<AccountId, u64>, BTreeSet<NonFungibleAsset>), TransactionRequestError>


oh - I forgot that to get incoming note assets we need to query the store. In this case, probably better to have this in the transactions module (rather than transaction request). And if we do this, we should probably move get_outgoing_assets back there for consistency.

Sounds good. I've moved it.

bobbinth

Looks good! Thank you! I left a few small comments inline. Once these are addressed, we can merge.

crates/rust-client/src/transactions/request.rs

crates/rust-client/src/transactions/mod.rs

…g txs

…add to do

igamigo

LGTM! Left a very minor non-blocking comment, feel free to disregard

igamigo · 2024-08-06T14:46:26Z

crates/rust-client/src/transactions/mod.rs

+        // Check if the account balance plus incoming assets is greater than or equal to the outgoing
+        // fungible assets
+        for (faucet_id, amount) in fungible_balance_map {
+            match account.vault().get_balance(faucet_id) {


nit: Maybe you can do get_balance(faucet_id).unwrap_or(0) (as you did below) here and you'd be able to get rid of the second match branch

igamigo · 2024-08-06T14:48:04Z

crates/rust-client/src/transactions/mod.rs

+    ) -> Result<(), ClientError> {
+        let (account, _) = maybe_await!(self.get_account(transaction_request.account_id()))?;
+        if account.is_faucet() {
+            // TODO(SantiagoPittella): Add faucet validations.


You probably were tracking this already but just in case: let's open an issue for this

I just created #468 ✅

Feel free to add whatever you want to the issue.

bobbinth

Looks good! Thank you!

One thing: let's create an issue to figure out if we want to make it so that own output notes are always a strict subset of expected output notes.

igamigo · 2024-08-06T16:56:19Z

Looks good! Thank you!

One thing: let's create an issue to figure out if we want to make it so that own output notes are always a strict subset of expected output notes.

Created #470 for this

bobbinth reviewed Jul 25, 2024

View reviewed changes

crates/rust-client/src/transactions/mod.rs Outdated Show resolved Hide resolved

bobbinth reviewed Jul 25, 2024

View reviewed changes

crates/rust-client/src/transactions/mod.rs Outdated Show resolved Hide resolved

igamigo reviewed Jul 29, 2024

View reviewed changes

crates/rust-client/src/transactions/mod.rs Outdated Show resolved Hide resolved

crates/rust-client/src/transactions/mod.rs Outdated Show resolved Hide resolved

crates/rust-client/src/transactions/mod.rs Outdated Show resolved Hide resolved

SantiagoPittella force-pushed the validations-for-transaction-requests branch from 78e175a to 44cffa1 Compare July 30, 2024 18:43

SantiagoPittella marked this pull request as ready for review July 30, 2024 19:44

SantiagoPittella linked an issue Jul 30, 2024 that may be closed by this pull request

feat: add basic validations for transaction requests #444

Closed

igamigo reviewed Jul 30, 2024

View reviewed changes

tomyrd reviewed Jul 30, 2024

View reviewed changes

tests/integration/common.rs Outdated Show resolved Hide resolved

SantiagoPittella force-pushed the validations-for-transaction-requests branch from 52a178f to 28f1871 Compare August 1, 2024 15:59

SantiagoPittella requested review from bobbinth, tomyrd and igamigo August 1, 2024 16:04

igamigo approved these changes Aug 1, 2024

View reviewed changes

crates/rust-client/src/transactions/mod.rs Outdated Show resolved Hide resolved

crates/rust-client/src/transactions/mod.rs Outdated Show resolved Hide resolved

crates/rust-client/src/transactions/mod.rs Outdated Show resolved Hide resolved

SantiagoPittella force-pushed the validations-for-transaction-requests branch from 9250b6a to d1e9047 Compare August 1, 2024 21:52

bobbinth requested changes Aug 1, 2024

View reviewed changes

bobbinth reviewed Aug 2, 2024

View reviewed changes

SantiagoPittella force-pushed the validations-for-transaction-requests branch 2 times, most recently from 4f1f228 to 03e1671 Compare August 5, 2024 14:38

SantiagoPittella requested a review from bobbinth August 5, 2024 14:39

bobbinth approved these changes Aug 5, 2024

View reviewed changes

SantiagoPittella added 9 commits August 6, 2024 11:04

refactor(transactions): add transaction validation before execution

3612c10

move validate_transaction function call

712c98d

rename function, add maybe_await

a101b2b

WIP: own notes and vec for non fungibles

f13eaa1

handle script template None option

207f95f

reorder imports

1830957

add missing maybe_await

426385e

add integration test for not enough balance tx, add helper for failin…

3de71ee

…g txs

lint & format

9336ce4

SantiagoPittella added 15 commits August 6, 2024 11:04

update CHANGELOG

b47dabb

use matches

b2bd788

wip

d50eca2

Use BTreeSet instead of Vec, functional approach

850aae7

remove lifetimes, refactor assets check

6d41693

rollback to .to_string in test

29b7bce

avoid vec, use getters for assets

65679d5

add faucet account validations

1e9ea15

fix format

e418886

move get_{outgoing,incoming}_assets methods to tx request

238e0b4

move get_{outgoing,incoming}_assets methods to transaction module

7d53c2b

fix: include unauthenticated notes in validation

02c956a

remove unused error

bae3f47

change validation methods visibility, remove validate_faucet method, …

303bfac

…add to do

abstract asset collection in function, add comments

f63c612

SantiagoPittella force-pushed the validations-for-transaction-requests branch from b42764e to f63c612 Compare August 6, 2024 14:05

igamigo approved these changes Aug 6, 2024

View reviewed changes

igamigo and others added 2 commits August 6, 2024 12:09

Set build script to use miden-node's next

0537f21

use unwrap_or instead of match

88ecd56

SantiagoPittella mentioned this pull request Aug 6, 2024

refactor(transaction): Validations for Faucets transaction requests. #468

Open

bobbinth approved these changes Aug 6, 2024

View reviewed changes

SantiagoPittella merged commit 3bdafca into next Aug 6, 2024
13 checks passed

SantiagoPittella deleted the validations-for-transaction-requests branch August 6, 2024 17:20

bobbinth mentioned this pull request Aug 6, 2024

feat: add basic validations for transaction requests #444

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(transaction): Validations for transaction requests #447

refactor(transaction): Validations for transaction requests #447

SantiagoPittella commented Jul 24, 2024

igamigo commented Jul 30, 2024

igamigo left a comment

bobbinth left a comment

bobbinth Aug 1, 2024

bobbinth Aug 1, 2024

bobbinth Aug 1, 2024

bobbinth Aug 1, 2024

bobbinth Aug 2, 2024

SantiagoPittella Aug 5, 2024

bobbinth left a comment

igamigo left a comment •

edited

Loading

igamigo Aug 6, 2024

igamigo Aug 6, 2024

SantiagoPittella Aug 6, 2024

SantiagoPittella Aug 6, 2024

bobbinth left a comment

igamigo commented Aug 6, 2024

refactor(transaction): Validations for transaction requests #447

refactor(transaction): Validations for transaction requests #447

Conversation

SantiagoPittella commented Jul 24, 2024

igamigo commented Jul 30, 2024

igamigo left a comment

Choose a reason for hiding this comment

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

bobbinth left a comment

Choose a reason for hiding this comment

igamigo left a comment • edited Loading

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

bobbinth left a comment

Choose a reason for hiding this comment

igamigo commented Aug 6, 2024

igamigo left a comment •

edited

Loading