Merged
Conversation
grjte
suggested changes
Dec 15, 2022
Contributor
grjte
left a comment
There was a problem hiding this comment.
Looks good! Most of my comments are minor but I think there's one issue that should be corrected
vlopes11
suggested changes
Dec 15, 2022
vlopes11
left a comment
There was a problem hiding this comment.
Looks good! A couple of suggestions. I think the safe arithmetic would be a critical part, specifically for the addition
6586579 to
7520f00
Compare
7520f00 to
d97a5c5
Compare
d97a5c5 to
03be522
Compare
hackaugusto
reviewed
Jan 16, 2023
Comment on lines
+34
to
+36
| /// - First the asset data is hashed. This compresses an asset of an arbitrary length to 4 field | ||
| /// elements. | ||
| /// - The result of step 1 is hashed together with the ID of the faucet which issued the asset. |
Contributor
There was a problem hiding this comment.
Is there a reason to not use hash(asset_data || faucet_id)? Do we want to blind the asset_data?
Contributor
Author
There was a problem hiding this comment.
Yes, but also I think it will make things a bit easier. i.e., to verify that the asset has been computed correctly, we don't need to hash the full asset data in the VM. Instead, we can hash it outside of the VM and in the VM we just hash data hash and faucet ID.
varun-doshi
added a commit
to varun-doshi/miden-base
that referenced
this pull request
Nov 4, 2025
varun-doshi
added a commit
to varun-doshi/miden-base
that referenced
this pull request
Nov 4, 2025
huitseeker
added a commit
that referenced
this pull request
Dec 28, 2025
…ree semantics ## Context The Plonky3 migration includes an updated PartialSmt implementation in miden-crypto that changes the semantics of "tracked" keys in empty trees. ## Semantic Change In the new PartialSmt implementation, a key is considered "tracked" if either: 1. Its merkle path was explicitly added via add_path/add_proof, OR 2. The path from the leaf to the root goes through empty subtrees that are provably empty (they equal the empty subtree root hash). Critically, in a COMPLETELY EMPTY tree (root == EMPTY_ROOT), ALL keys satisfy condition #2 and are therefore implicitly tracked. This is correct cryptographic behavior: in an empty tree, all leaves are provably empty via zero hash computations, so there's no need to explicitly track them. ## Test Update The test `upsert_state_commitments_fails_on_untracked_key` was using `PartialAccountTree::default()` which creates an empty tree with EMPTY_ROOT. Under the new semantics, all keys are implicitly tracked in this tree, so inserting an "untracked" key would succeed rather than fail. The fix: Initialize the partial tree with a non-empty root so that keys are NOT implicitly tracked, allowing us to properly test the UntrackedAccountId error path. ## Related Code See miden-crypto/src/merkle/smt/partial/mod.rs:368-370: ```rust // Empty tree - all leaves implicitly trackable if self.root == Self::EMPTY_ROOT { return Some(SmtLeaf::new_empty(leaf_index)); } ```
huitseeker
added a commit
that referenced
this pull request
Dec 28, 2025
…ree semantics ## Context The Plonky3 migration includes an updated PartialSmt implementation in miden-crypto that changes the semantics of "tracked" keys in empty trees. ## Semantic Change In the new PartialSmt implementation, a key is considered "tracked" if either: 1. Its merkle path was explicitly added via add_path/add_proof, OR 2. The path from the leaf to the root goes through empty subtrees that are provably empty (they equal the empty subtree root hash). Critically, in a COMPLETELY EMPTY tree (root == EMPTY_ROOT), ALL keys satisfy condition #2 and are therefore implicitly tracked. This is correct cryptographic behavior: in an empty tree, all leaves are provably empty via zero hash computations, so there's no need to explicitly track them. ## Test Update The test `upsert_state_commitments_fails_on_untracked_key` was using `PartialAccountTree::default()` which creates an empty tree with EMPTY_ROOT. Under the new semantics, all keys are implicitly tracked in this tree, so inserting an "untracked" key would succeed rather than fail. The fix: Initialize the partial tree with a non-empty root so that keys are NOT implicitly tracked, allowing us to properly test the UntrackedAccountId error path. ## Related Code See miden-crypto/src/merkle/smt/partial/mod.rs:368-370: ```rust // Empty tree - all leaves implicitly trackable if self.root == Self::EMPTY_ROOT { return Some(SmtLeaf::new_empty(leaf_index)); } ```
huitseeker
added a commit
that referenced
this pull request
Dec 28, 2025
…ree semantics ## Context The Plonky3 migration includes an updated PartialSmt implementation in miden-crypto that changes the semantics of "tracked" keys in empty trees. ## Semantic Change In the new PartialSmt implementation, a key is considered "tracked" if either: 1. Its merkle path was explicitly added via add_path/add_proof, OR 2. The path from the leaf to the root goes through empty subtrees that are provably empty (they equal the empty subtree root hash). Critically, in a COMPLETELY EMPTY tree (root == EMPTY_ROOT), ALL keys satisfy condition #2 and are therefore implicitly tracked. This is correct cryptographic behavior: in an empty tree, all leaves are provably empty via zero hash computations, so there's no need to explicitly track them. ## Test Update The test `upsert_state_commitments_fails_on_untracked_key` was using `PartialAccountTree::default()` which creates an empty tree with EMPTY_ROOT. Under the new semantics, all keys are implicitly tracked in this tree, so inserting an "untracked" key would succeed rather than fail. The fix: Initialize the partial tree with a non-empty root so that keys are NOT implicitly tracked, allowing us to properly test the UntrackedAccountId error path. ## Related Code See miden-crypto/src/merkle/smt/partial/mod.rs:368-370: ```rust // Empty tree - all leaves implicitly trackable if self.root == Self::EMPTY_ROOT { return Some(SmtLeaf::new_empty(leaf_index)); } ```
huitseeker
added a commit
that referenced
this pull request
Dec 28, 2025
…ree semantics ## Context The Plonky3 migration includes an updated PartialSmt implementation in miden-crypto that changes the semantics of "tracked" keys in empty trees. ## Semantic Change In the new PartialSmt implementation, a key is considered "tracked" if either: 1. Its merkle path was explicitly added via add_path/add_proof, OR 2. The path from the leaf to the root goes through empty subtrees that are provably empty (they equal the empty subtree root hash). Critically, in a COMPLETELY EMPTY tree (root == EMPTY_ROOT), ALL keys satisfy condition #2 and are therefore implicitly tracked. This is correct cryptographic behavior: in an empty tree, all leaves are provably empty via zero hash computations, so there's no need to explicitly track them. ## Test Update The test `upsert_state_commitments_fails_on_untracked_key` was using `PartialAccountTree::default()` which creates an empty tree with EMPTY_ROOT. Under the new semantics, all keys are implicitly tracked in this tree, so inserting an "untracked" key would succeed rather than fail. The fix: Initialize the partial tree with a non-empty root so that keys are NOT implicitly tracked, allowing us to properly test the UntrackedAccountId error path. ## Related Code See miden-crypto/src/merkle/smt/partial/mod.rs:368-370: ```rust // Empty tree - all leaves implicitly trackable if self.root == Self::EMPTY_ROOT { return Some(SmtLeaf::new_empty(leaf_index)); } ```
huitseeker
added a commit
that referenced
this pull request
Dec 28, 2025
…ree semantics ## Context The Plonky3 migration includes an updated PartialSmt implementation in miden-crypto that changes the semantics of "tracked" keys in empty trees. ## Semantic Change In the new PartialSmt implementation, a key is considered "tracked" if either: 1. Its merkle path was explicitly added via add_path/add_proof, OR 2. The path from the leaf to the root goes through empty subtrees that are provably empty (they equal the empty subtree root hash). Critically, in a COMPLETELY EMPTY tree (root == EMPTY_ROOT), ALL keys satisfy condition #2 and are therefore implicitly tracked. This is correct cryptographic behavior: in an empty tree, all leaves are provably empty via zero hash computations, so there's no need to explicitly track them. ## Test Update The test `upsert_state_commitments_fails_on_untracked_key` was using `PartialAccountTree::default()` which creates an empty tree with EMPTY_ROOT. Under the new semantics, all keys are implicitly tracked in this tree, so inserting an "untracked" key would succeed rather than fail. The fix: Initialize the partial tree with a non-empty root so that keys are NOT implicitly tracked, allowing us to properly test the UntrackedAccountId error path. ## Related Code See miden-crypto/src/merkle/smt/partial/mod.rs:368-370: ```rust // Empty tree - all leaves implicitly trackable if self.root == Self::EMPTY_ROOT { return Some(SmtLeaf::new_empty(leaf_index)); } ```
huitseeker
added a commit
that referenced
this pull request
Dec 28, 2025
…ree semantics ## Context The Plonky3 migration includes an updated PartialSmt implementation in miden-crypto that changes the semantics of "tracked" keys in empty trees. ## Semantic Change In the new PartialSmt implementation, a key is considered "tracked" if either: 1. Its merkle path was explicitly added via add_path/add_proof, OR 2. The path from the leaf to the root goes through empty subtrees that are provably empty (they equal the empty subtree root hash). Critically, in a COMPLETELY EMPTY tree (root == EMPTY_ROOT), ALL keys satisfy condition #2 and are therefore implicitly tracked. This is correct cryptographic behavior: in an empty tree, all leaves are provably empty via zero hash computations, so there's no need to explicitly track them. ## Test Update The test `upsert_state_commitments_fails_on_untracked_key` was using `PartialAccountTree::default()` which creates an empty tree with EMPTY_ROOT. Under the new semantics, all keys are implicitly tracked in this tree, so inserting an "untracked" key would succeed rather than fail. The fix: Initialize the partial tree with a non-empty root so that keys are NOT implicitly tracked, allowing us to properly test the UntrackedAccountId error path. ## Related Code See miden-crypto/src/merkle/smt/partial/mod.rs:368-370: ```rust // Empty tree - all leaves implicitly trackable if self.root == Self::EMPTY_ROOT { return Some(SmtLeaf::new_empty(leaf_index)); } ```
huitseeker
added a commit
that referenced
this pull request
Jan 6, 2026
…ree semantics ## Context The Plonky3 migration includes an updated PartialSmt implementation in miden-crypto that changes the semantics of "tracked" keys in empty trees. ## Semantic Change In the new PartialSmt implementation, a key is considered "tracked" if either: 1. Its merkle path was explicitly added via add_path/add_proof, OR 2. The path from the leaf to the root goes through empty subtrees that are provably empty (they equal the empty subtree root hash). Critically, in a COMPLETELY EMPTY tree (root == EMPTY_ROOT), ALL keys satisfy condition #2 and are therefore implicitly tracked. This is correct cryptographic behavior: in an empty tree, all leaves are provably empty via zero hash computations, so there's no need to explicitly track them. ## Test Update The test `upsert_state_commitments_fails_on_untracked_key` was using `PartialAccountTree::default()` which creates an empty tree with EMPTY_ROOT. Under the new semantics, all keys are implicitly tracked in this tree, so inserting an "untracked" key would succeed rather than fail. The fix: Initialize the partial tree with a non-empty root so that keys are NOT implicitly tracked, allowing us to properly test the UntrackedAccountId error path. ## Related Code See miden-crypto/src/merkle/smt/partial/mod.rs:368-370: ```rust // Empty tree - all leaves implicitly trackable if self.root == Self::EMPTY_ROOT { return Some(SmtLeaf::new_empty(leaf_index)); } ```
huitseeker
added a commit
that referenced
this pull request
Jan 28, 2026
…ree semantics ## Context The Plonky3 migration includes an updated PartialSmt implementation in miden-crypto that changes the semantics of "tracked" keys in empty trees. ## Semantic Change In the new PartialSmt implementation, a key is considered "tracked" if either: 1. Its merkle path was explicitly added via add_path/add_proof, OR 2. The path from the leaf to the root goes through empty subtrees that are provably empty (they equal the empty subtree root hash). Critically, in a COMPLETELY EMPTY tree (root == EMPTY_ROOT), ALL keys satisfy condition #2 and are therefore implicitly tracked. This is correct cryptographic behavior: in an empty tree, all leaves are provably empty via zero hash computations, so there's no need to explicitly track them. ## Test Update The test `upsert_state_commitments_fails_on_untracked_key` was using `PartialAccountTree::default()` which creates an empty tree with EMPTY_ROOT. Under the new semantics, all keys are implicitly tracked in this tree, so inserting an "untracked" key would succeed rather than fail. The fix: Initialize the partial tree with a non-empty root so that keys are NOT implicitly tracked, allowing us to properly test the UntrackedAccountId error path. ## Related Code See miden-crypto/src/merkle/smt/partial/mod.rs:368-370: ```rust // Empty tree - all leaves implicitly trackable if self.root == Self::EMPTY_ROOT { return Some(SmtLeaf::new_empty(leaf_index)); } ```
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR implements
Assetand related objects.One open questions: should we allow non-fungible assets to have amounts set to 0?