Tests cleanup 2 #771

hackaugusto · 2024-06-25T19:01:32Z

builds on #764

hackaugusto · 2024-06-25T19:05:34Z

miden-tx/src/testing/utils.rs


 // TEST HELPERS
 // ================================================================================================

 pub fn consumed_note_data_ptr(note_idx: u32) -> memory::MemoryAddress {
    memory::CONSUMED_NOTE_DATA_SECTION_OFFSET + note_idx * memory::NOTE_MEM_SIZE
 }
-
-pub fn mock_executed_tx(asset_preservation: AssetPreservationStatus) -> ExecutedTransaction {


This was a very weird implementation, the dummy program push.0 drop had nothing to do with the actual tests, which always had custom code. also most tests didn't need the final account state (only one used it).

so in order to make the test setup and the actual test consistent with one another, I removed this in favor of just using the context. It is less code, less indirection, less inconsistencies, and the same results.

hackaugusto · 2024-06-25T19:06:52Z

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

-/// Loads epilogue file and returns the complete code formatted as
-/// "{imports}{epilogue_code}{code}"`
-#[cfg(feature = "std")]
-fn insert_epilogue(imports: &str, code: &str) -> String {


I'm not sure what this was trying to accomplish. Replaced it with a simpler use.miden... instead.

Originally I replaced the function with something very similar as I didn't fully understand why it was there. I think only a few tests made use of it so this is much better.

hackaugusto · 2024-06-25T19:07:48Z

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

-/// Loads epilogue file and returns the complete code formatted as
-/// "{prologue_code}{code}"`
-#[cfg(feature = "std")]
-fn insert_prologue(code: &str) -> String {


This too wasn't really necessary, and using use statements was sufficient for the tests.

hackaugusto · 2024-06-25T19:09:58Z

objects/src/accounts/account_id.rs

@@ -308,6 +313,23 @@ impl From<AccountId> for LeafIndex<ACCOUNT_TREE_DEPTH> {
 // CONVERSIONS TO ACCOUNT ID
 // ================================================================================================

+pub const fn account_id_from_felt(value: Felt) -> Result<AccountId, AccountError> {


I added a few const functions. My goal was to remove the duplication in the test context, when setting up account ids and assets. The issue is that unwrap is no const yet, so I ended up leaving the code changes behind, once unwrap const is declared stable this can be used.

hackaugusto · 2024-06-25T19:12:32Z

miden-tx/src/testing/tx_context.rs

+            .unwrap()
+    }
+
+    pub fn with_mock_notes_too_few_input(mut self) -> Self {


Just like the accounts, I broke up the scenarios into different methods. This makes it easier to modify the scenarios, since the function body contains only the code relevant to the scenario at hand. I'm going to stop here, since at this stage it is easy to add a input_note_transfer_delayed_authentication of sorts, which is what I need for testing, but I think these methods need better names (they are completely cryptic at the moment), some documentation, and some could even be removed because they are not used more than once.

hackaugusto · 2024-06-25T19:13:14Z

miden-tx/src/testing/tx_context.rs

+        // ACCOUNT IDS
+        // --------------------------------------------------------------------------------------------
+        let sender = AccountId::try_from(ACCOUNT_ID_SENDER).unwrap();
+        let faucet_id_1 = AccountId::try_from(ACCOUNT_ID_FUNGIBLE_FAUCET_ON_CHAIN_1).unwrap();
+        let faucet_id_2 = AccountId::try_from(ACCOUNT_ID_FUNGIBLE_FAUCET_ON_CHAIN_2).unwrap();
+        let faucet_id_3 = AccountId::try_from(ACCOUNT_ID_FUNGIBLE_FAUCET_ON_CHAIN_3).unwrap();
+
+        // ASSETS
+        // --------------------------------------------------------------------------------------------
+        let fungible_asset_1: Asset =
+            FungibleAsset::new(faucet_id_1, CONSUMED_ASSET_1_AMOUNT).unwrap().into();
+        let fungible_asset_2: Asset =
+            FungibleAsset::new(faucet_id_2, CONSUMED_ASSET_2_AMOUNT).unwrap().into();
+        let fungible_asset_3: Asset =
+            FungibleAsset::new(faucet_id_3, CONSUMED_ASSET_3_AMOUNT).unwrap().into();


These are the constants I was trying to define using const functions. Unfortunately because of the validation/unwrap it is not possible yet.

bobbinth · 2024-06-26T07:25:34Z

@hackaugusto - I merged #764 first and it screwed up commits in this PR. Could you rebase this PR from next?

hackaugusto · 2024-06-26T12:39:31Z

miden-tx/src/testing/tx_context.rs

+    fn input_note_simple(
+        &mut self,
+        sender: AccountId,
+        assets: impl IntoIterator<Item = Asset>,
+        inputs: impl IntoIterator<Item = Felt>,
+    ) -> Note {
+        NoteBuilder::new(sender, ChaCha20Rng::from_seed(self.rng.gen()))
+            .note_inputs(inputs)
+            .unwrap()
+            .add_assets(assets)
+            .build(&self.assembler)
+            .unwrap()
+    }


NOTE: I'm not sure any of these notes are valid. The default code of a note is empty, literally begin end, meaning the assets are not added to the account running the tx kernel nor added to a new output note. These should fail on the epilogue check, when the asset vault from the inputs+account is compared against the outputs+account. I believe tests are not failing because we are not testing the main.masm.

With that said, I didn't dig through further. I would need to read all the tests, and figure out how they are using the notes, to know what the fix is (that is, if my guess above is even correct). Figuring out what these functions are actually used for needs more work.

hackaugusto · 2024-06-26T12:43:16Z

miden-tx/src/testing/tx_context.rs

+    fn input_note_with_one_output_note(
+        &mut self,
+        sender: AccountId,
+        assets: impl IntoIterator<Item = Asset>,
+        inputs: impl IntoIterator<Item = Felt>,
+        output: &Note,
+    ) -> Note {
+        let code = format!(
+            "
+            begin
+                # NOTE
+                # ---------------------------------------------------------------------------------
+                push.{recipient}
+                push.{PUBLIC_NOTE}
+                push.{aux}
+                push.{tag}
+                call.{ACCOUNT_CREATE_NOTE_MAST_ROOT}
+
+                push.{asset} movup.4
+                call.{ACCOUNT_ADD_ASSET_TO_NOTE_MAST_ROOT}
+                dropw dropw dropw
+            end
+            ",
+            PUBLIC_NOTE = NoteType::Public as u8,
+            recipient = prepare_word(&output.recipient().digest()),
+            aux = output.metadata().aux(),
+            tag = output.metadata().tag(),
+            asset = prepare_assets(output.assets())[0],
+        );
+
+        NoteBuilder::new(sender, ChaCha20Rng::from_seed(self.rng.gen()))
+            .note_inputs(inputs)
+            .unwrap()
+            .add_assets(assets)
+            .code(code)
+            .build(&self.assembler)
+            .unwrap()
+    }


NOTE: I'm not sure what is the point of these notes. It looks like it should be a tx_script instead? There are quite a bit of weirdness to these:

the inputs are never used

the input assets are never consumed

the output could have more than one asset, which is not handled here

These are largely also issues with output_notes_data_procedure.

Here too more work is needed to figure out how to handle these.

bobbinth

Looks good! Thank you! I didn't do a super in-depth review - so, it would be good to get another review from @igamigo.

bobbinth · 2024-06-30T23:38:31Z

objects/src/accounts/account_id.rs

@@ -40,24 +40,29 @@ pub enum AccountType {
    RegularAccountUpdatableCode = REGULAR_ACCOUNT_UPDATABLE_CODE,
 }

+pub const fn account_type_from_u64(value: u64) -> AccountType {


Let's add doc comments to this function. Specifically, we should mention that account type is expected to be in the two most significant bits of the passed-in value.

bobbinth · 2024-06-30T23:59:34Z

objects/src/accounts/account_id.rs

@@ -308,6 +313,23 @@ impl From<AccountId> for LeafIndex<ACCOUNT_TREE_DEPTH> {
 // CONVERSIONS TO ACCOUNT ID
 // ================================================================================================

+pub const fn account_id_from_felt(value: Felt) -> Result<AccountId, AccountError> {


Let's add a doc comment to this function (could be the same as on lines 336 - 342 below).

igamigo

LGTM, all changes make sense :)

igamigo · 2024-07-01T21:43:30Z

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

-/// Loads epilogue file and returns the complete code formatted as
-/// "{imports}{epilogue_code}{code}"`
-#[cfg(feature = "std")]
-fn insert_epilogue(imports: &str, code: &str) -> String {


Originally I replaced the function with something very similar as I didn't fully understand why it was there. I think only a few tests made use of it so this is much better.

hackaugusto · 2024-07-03T14:26:32Z

merged the PR since it had two separate approvals, and it was starting to get conflicts

* tests: output_notes_data_procedure takes &[Note] * tests: removed mock_executed_tx * tests: removed insert_epilogue * tests: removed insert_prologue * tests: split mock_notes utility into two * chore: removed unecessary constructors * chore: removed unecessary constructors * tests: added const fungible asset constructor * tests: builder accepting multiple assets * tests: builder accepting intoiterator * tests: method to create individual output notes * tests: method to create input note wich creates two output notes * tests: method to create input note wich creates one output note * tests: method to create simple transfer note * tests: method to create simple transfer note * tests: split notes scenarios into many methods * docs: added docs to account id utils

hackaugusto requested review from igamigo and bobbinth June 25, 2024 19:01

hackaugusto commented Jun 25, 2024

View reviewed changes

hackaugusto force-pushed the hacka-tests-cleanup-2 branch 2 times, most recently from d3ce3ec to 7b630cd Compare June 25, 2024 19:40

hackaugusto force-pushed the hacka-tests-cleanup-2 branch from 7b630cd to e3dada6 Compare June 26, 2024 12:32

hackaugusto commented Jun 26, 2024

View reviewed changes

bobbinth approved these changes Jul 1, 2024

View reviewed changes

hackaugusto force-pushed the hacka-tests-cleanup-2 branch from e3dada6 to 273d0c0 Compare July 2, 2024 14:10

igamigo approved these changes Jul 2, 2024

View reviewed changes

hackaugusto mentioned this pull request Jul 2, 2024

Bugfix allow account code change #784

Closed

hackaugusto added 12 commits July 3, 2024 16:08

tests: output_notes_data_procedure takes &[Note]

c52f830

tests: removed mock_executed_tx

1419c27

tests: removed insert_epilogue

41f0713

tests: removed insert_prologue

3ffaaa9

tests: split mock_notes utility into two

f0cf4de

chore: removed unecessary constructors

0edd637

chore: removed unecessary constructors

f1e1847

tests: added const fungible asset constructor

c2eb9e7

tests: builder accepting multiple assets

acb83e1

tests: builder accepting intoiterator

d710117

tests: method to create individual output notes

79cc343

tests: method to create input note wich creates two output notes

55130f3

hackaugusto added 5 commits July 3, 2024 16:08

tests: method to create input note wich creates one output note

43c9a64

tests: method to create simple transfer note

a3b0d14

tests: method to create simple transfer note

af165c1

tests: split notes scenarios into many methods

ae99bd8

docs: added docs to account id utils

04e42d2

hackaugusto force-pushed the hacka-tests-cleanup-2 branch from 273d0c0 to 04e42d2 Compare July 3, 2024 14:10

hackaugusto merged commit f25bf32 into next Jul 3, 2024
12 checks passed

hackaugusto deleted the hacka-tests-cleanup-2 branch July 3, 2024 14:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tests cleanup 2 #771

Tests cleanup 2 #771

hackaugusto commented Jun 25, 2024

hackaugusto Jun 25, 2024

hackaugusto Jun 25, 2024

igamigo Jul 1, 2024

hackaugusto Jun 25, 2024

hackaugusto Jun 25, 2024 •

edited

Loading

hackaugusto Jun 25, 2024

hackaugusto Jun 25, 2024

bobbinth commented Jun 26, 2024

hackaugusto Jun 26, 2024

hackaugusto Jun 26, 2024

bobbinth left a comment

bobbinth Jun 30, 2024

hackaugusto Jul 2, 2024

bobbinth Jun 30, 2024

hackaugusto Jul 2, 2024

igamigo left a comment

igamigo Jul 1, 2024

hackaugusto commented Jul 3, 2024

Tests cleanup 2 #771

Tests cleanup 2 #771

Conversation

hackaugusto commented Jun 25, 2024

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

hackaugusto Jun 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobbinth commented Jun 26, 2024

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

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

igamigo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hackaugusto commented Jul 3, 2024

hackaugusto Jun 25, 2024 •

edited

Loading