test: Add tests and refactor/document structs #871

igamigo · 2024-09-12T13:07:01Z

Closes #446

igamigo · 2024-11-04T15:08:53Z

miden-tx/src/tests/kernel_tests/test_tx.rs


-/// Assert that mock account's `get_item` procedure has index 6
+/// Assert that mock account's `get_item` procedure has index 8
 #[test]
 fn test_check_get_item_index() {


I replaced calling foreign procedures by root in favor of using procref. Maybe we don't want this change, but if we do, I'm not sure we need this test anymore.

Sounds reasonable to me. I'm not sure if the intent of the test was to use the MAST root deliberately to test something, but perhaps @Fumuran can clarify.

Excuse me for the late reply, I missed the notification.
I think using the procref is much better, for some reason I was so fixated on obtaining the procedure's hash, that I forgot that we can just obtain it with procref. This procedure is necessary only for checking the correct index of the get_item procedure, but since now we use procref this test is not needed anymore. I'll remove it in my test refactoring PR.

PhilippGackstatter

Nice improvement of replacing quite some test code by using the MockChain.

Not a full review yet, just some preliminary comments.

miden-tx/src/testing/mock_chain/mod.rs

miden-tx/src/testing/tx_context/builder.rs

miden-tx/src/tests/kernel_tests/test_account.rs

PhilippGackstatter · 2024-11-07T16:12:35Z

miden-tx/src/testing/mock_chain/mod.rs

+///
+/// ```
+/// let mut mock_chain = MockChain::default();
+/// let sender = mock_chain.add_new_wallet(Auth::BasicAuth, vec![asset]);  // Add a wallet with assets


Just FYI: This is no longer allowed in the new AccountBuilder after #952 since new accounts are not allowed to have assets. This can probably just be changed to be an existing account.

I think to be a valid example this still needs to be changed to mock_chain.add_existing_wallet since new wallets are not allowed to have assets.

Removed the ability of setting assets and changed the examples

PhilippGackstatter · 2024-11-07T16:14:04Z

miden-tx/src/tests/kernel_tests/test_tx.rs


-/// Assert that mock account's `get_item` procedure has index 6
+/// Assert that mock account's `get_item` procedure has index 8
 #[test]
 fn test_check_get_item_index() {


Sounds reasonable to me. I'm not sure if the intent of the test was to use the MAST root deliberately to test something, but perhaps @Fumuran can clarify.

miden-tx/src/tests/kernel_tests/test_tx.rs

miden-tx/tests/integration/scripts/faucet.rs

PhilippGackstatter

Looks good to me!

I'll wait after the rebase to approve.

Mainly the comment about asserting the error conditions rather than just is_err() would be nice, other than that just small comments.

miden-tx/tests/integration/scripts/p2id.rs

miden-tx/tests/integration/wallet/mod.rs

igamigo · 2024-11-12T20:36:02Z

Apologies for the delay, was feeling a bit under the weather but should be rebased with all comments addressed now

PhilippGackstatter

Looks great - thanks for the updates! Hope your feeling better soon, too.

Just optional comments, so I'll approve and I think you can merge when you feel it's ready.

PhilippGackstatter · 2024-11-13T07:30:24Z

miden-tx/src/testing/mock_chain/mod.rs

+///
+/// ```
+/// let mut mock_chain = MockChain::default();
+/// let sender = mock_chain.add_new_wallet(Auth::BasicAuth, vec![asset]);  // Add a wallet with assets


I think to be a valid example this still needs to be changed to mock_chain.add_existing_wallet since new wallets are not allowed to have assets.

PhilippGackstatter · 2024-11-13T07:38:27Z

miden-tx/src/testing/mock_chain/mod.rs

+        // Note: data is stored as [a0, a1, a2, a3] but loaded onto the stack as
+        // [a3, a2, a1, a0, ...]
+        let metadata = [
+            Felt::new(max_supply),
+            Felt::new(10u64),
+            TokenSymbol::new(token_symbol).unwrap().into(),
+            Felt::ZERO,
+        ];
+
+        let faucet_component = AccountComponent::new(basic_fungible_faucet_library(), vec![StorageSlot::Value(metadata)])
+            .expect("basic fungible faucet component should satisfy the requirements of a valid account component")
+            .with_supported_type(AccountType::FungibleFaucet);


You should be able to use the BasicFungibleFaucet component here as on next to write this more concisely:

https://github.com/0xPolygonMiden/miden-base/blame/31b1d7a376fed44308606cdafe9cef9aa7994849/miden-tx/src/testing/mock_chain.rs#L356-L367

PhilippGackstatter · 2024-11-13T07:47:07Z

miden-tx/src/testing/mock_chain/mod.rs

+        let (mut account, seed, authenticator) = match auth_method {
+            Auth::BasicAuth => {


There is some duplication in this function and add_from_account_builder that we could deduplicate I think.
On the other hand, I will refactor add_from_account_builder anyway (#952 (comment)) so we can just leave it as-is and I'll look at it in the follow-up PR.

I forgot to do a second pass on this function. I mostly copied code because I didn't find a way to re-set the first slot (for the total issuance) in a way that fit the context of the function (ie, from AccountBuilder before passing it on to add_from_account_builder())

miden-tx/tests/integration/main.rs

PhilippGackstatter · 2024-11-13T08:06:47Z

miden-tx/tests/integration/scripts/faucet.rs

+    assert_transaction_executor_error!(
+        tx,
+        ERR_FUNGIBLE_ASSET_DISTRIBUTE_WOULD_CAUSE_MAX_SUPPLY_TO_BE_EXCEEDED
+    );


This is really nice now, thanks for adding this!

objects/src/testing/assets.rs

PhilippGackstatter · 2024-11-13T08:21:14Z

objects/src/testing/assets.rs

        let non_fungible_asset_details = NonFungibleAssetDetails::new(
-            AccountId::try_from(account_id).unwrap(),
+            AccountId::try_from(ACCOUNT_ID_NON_FUNGIBLE_FAUCET_ON_CHAIN).unwrap(),


Suggested change

AccountId::try_from(ACCOUNT_ID_NON_FUNGIBLE_FAUCET_ON_CHAIN).unwrap(),

Self::mock_issuer(),

Totally optional.

PhilippGackstatter · 2024-11-13T08:23:04Z

objects/src/testing/assets.rs

@@ -106,4 +118,9 @@ impl FungibleAsset {
            .expect("asset is valid"),
        )
    }
+
+    /// Returns a mocked asset account ID ([ACCOUNT_ID_FUNGIBLE_FAUCET_ON_CHAIN]).


Suggested change

/// Returns a mocked asset account ID ([ACCOUNT_ID_FUNGIBLE_FAUCET_ON_CHAIN]).

/// Returns the account ID of the issuer of [`FungibleAsset::mock()`] ([ACCOUNT_ID_FUNGIBLE_FAUCET_ON_CHAIN]).

PhilippGackstatter · 2024-11-13T08:29:21Z

objects/src/testing/storage.rs

+    /// Configures storage slots for a wallet account with authentication.
+    pub fn with_wallet_storage(&mut self, public_key: PublicKey) -> &mut Self {
+        self.add_slot(StorageSlot::Value(public_key.into()));
+
+        self
+    }


The entire AccountStorageBuilder is now unused. Should we remove it?
I think even in cases where we want to build custom storage for tests we should probably construct it with AccountComponents and if that is insufficient modify it on AccountStorage directly like you did when setting total issuance in a faucet's reserved slot - what do you think?

Yeah, I thought of completely removing this but was not sure anything else from the outside was currently relying on this. I'll remove it.

bobbinth · 2024-11-15T00:53:04Z

@igamigo - since this is merged, we should be able to close a couple of related issues, right?

igamigo force-pushed the igamigo-docs-tests branch 2 times, most recently from 6278299 to 5a04926 Compare September 16, 2024 04:39

igamigo marked this pull request as ready for review September 16, 2024 14:57

igamigo added the no changelog This PR does not require an entry in the `CHANGELOG.md` file label Sep 16, 2024

igamigo mentioned this pull request Sep 17, 2024

Set to 1 all faucet account storage offsets #875

Merged

igamigo requested a review from bobbinth September 22, 2024 23:36

This was referenced Oct 8, 2024

fix: Faucet storage offset validation code #911

Merged

Use new faucets in tests #912

Open

igamigo commented Nov 4, 2024

View reviewed changes

bobbinth mentioned this pull request Nov 5, 2024

Introduce a public AccountBuilder #952

Merged

bobbinth requested a review from PhilippGackstatter November 5, 2024 20:05

PhilippGackstatter reviewed Nov 7, 2024

View reviewed changes

PhilippGackstatter reviewed Nov 8, 2024

View reviewed changes

PhilippGackstatter mentioned this pull request Nov 11, 2024

Revisit Account::initialize_from_components #949

Closed

igamigo requested a review from PhilippGackstatter November 12, 2024 20:33

PhilippGackstatter approved these changes Nov 13, 2024

View reviewed changes

PhilippGackstatter mentioned this pull request Nov 13, 2024

Revisit Account::initialize_from_components and add AccountBuilder::build_existing #969

Merged

igamigo force-pushed the igamigo-docs-tests branch from 8b0caf3 to 348a905 Compare November 13, 2024 18:48

test: Add tests, docs

8cf400d

igamigo force-pushed the igamigo-docs-tests branch from 348a905 to 8cf400d Compare November 13, 2024 18:50

igamigo merged commit ffc0fd9 into next Nov 14, 2024
9 checks passed

igamigo deleted the igamigo-docs-tests branch November 14, 2024 20:19

igamigo mentioned this pull request Nov 15, 2024

Create 2 transaction tests that consume and output notes #446

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: Add tests and refactor/document structs #871

test: Add tests and refactor/document structs #871

igamigo commented Sep 12, 2024

igamigo Nov 4, 2024

PhilippGackstatter Nov 7, 2024

Fumuran Nov 15, 2024

PhilippGackstatter left a comment

PhilippGackstatter Nov 7, 2024

PhilippGackstatter Nov 13, 2024

igamigo Nov 13, 2024

PhilippGackstatter Nov 7, 2024

PhilippGackstatter left a comment

igamigo commented Nov 12, 2024

PhilippGackstatter left a comment

PhilippGackstatter Nov 13, 2024

PhilippGackstatter Nov 13, 2024

PhilippGackstatter Nov 13, 2024

igamigo Nov 13, 2024

PhilippGackstatter Nov 13, 2024

PhilippGackstatter Nov 13, 2024

PhilippGackstatter Nov 13, 2024

PhilippGackstatter Nov 13, 2024

igamigo Nov 13, 2024

bobbinth commented Nov 15, 2024

		let (mut account, seed, authenticator) = match auth_method {
		Auth::BasicAuth => {

	AccountId::try_from(ACCOUNT_ID_NON_FUNGIBLE_FAUCET_ON_CHAIN).unwrap(),
	Self::mock_issuer(),

	/// Returns a mocked asset account ID ([ACCOUNT_ID_FUNGIBLE_FAUCET_ON_CHAIN]).
	/// Returns the account ID of the issuer of [`FungibleAsset::mock()`] ([ACCOUNT_ID_FUNGIBLE_FAUCET_ON_CHAIN]).

test: Add tests and refactor/document structs #871

test: Add tests and refactor/document structs #871

Conversation

igamigo commented Sep 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PhilippGackstatter 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

PhilippGackstatter left a comment

Choose a reason for hiding this comment

igamigo commented Nov 12, 2024

PhilippGackstatter 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

bobbinth commented Nov 15, 2024