feat: adapt the layout of Assets to the double word representation#2437
feat: adapt the layout of Assets to the double word representation#2437PhilippGackstatter wants to merge 39 commits intopgackst-user-asset-key-valuefrom
Assets to the double word representation#2437Conversation
| /// # Fungible assets | ||
| /// | ||
| /// - A fungible asset's data layout is: `[amount, 0, faucet_id_suffix, faucet_id_prefix]`. | ||
| /// - A fungible asset's value layout is: `[amount, 0, 0, 0]`. |
There was a problem hiding this comment.
nit:
| /// - A fungible asset's value layout is: `[amount, 0, 0, 0]`. | |
| /// - A fungible asset's value layout is: `[amount, 0, 0, 0]`. |
| impl Ord for Asset { | ||
| fn cmp(&self, other: &Self) -> core::cmp::Ordering { | ||
| self.vault_key().cmp(&other.vault_key()) | ||
| } | ||
| } |
There was a problem hiding this comment.
Is this fine? A fungible pair ofAsset can be != since it's compared by value (it derives Eq and PartialEq) but it would evaluate to Ordering::Equal becuase it's based on the asset's vault key, which I think is not desired since Eq and Ord are expected to be consistent. Maybe the amount should break the tie in the case of fungible assets? Although not sure what the expected use of this is; maybe documenting it better is enough.
| /// removed. If the final amount of the asset is zero, the asset is removed from the vault. | ||
| /// | ||
| /// # Errors | ||
| /// - The asset is not found in the vault. |
There was a problem hiding this comment.
This does not seem to be the case with the latest changes. Should it be? It might be better in order to have more precise error messages.
| @@ -108,57 +114,33 @@ impl NonFungibleAsset { | |||
| /// asset and a fungible asset, as the former's vault key always has the fungible bit set to `0` | |||
| /// and the latter's vault key always has the bit set to `1`. | |||
| pub fn vault_key(&self) -> AssetVaultKey { | |||
There was a problem hiding this comment.
I think this might require new docs
Refactors the layout of fungible and non-fungible assets. The main benefit for now is that non-fungible assets include the full account ID. The other good news is that this PR removes the last
unsafecode from miden base 😅.Layout Changes
For direct comparison, the previous layout was:
[amount, 0, faucet_id_suffix, faucet_id_prefix].[0, 0, faucet_id_suffix, faucet_id_prefix].[hash0, hash1, hash2, faucet_id_prefix].[faucet_id_prefix, hash1, hash2, hash0'].The new layout is:
[amount, 0, 0, 0].[0, 0, faucet_id_suffix, faucet_id_prefix].[hash0, hash1, hash2, hash3].[hash0, hash1, faucet_id_suffix, faucet_id_prefix].The most notable change is that non-fungible assets now contain the full
AccountIdinstead of just theAccountIdPrefix. I believe this removes the last reason we had that account ID prefixes needed to be unique in the account tree. So, we could now consider lifting this restriction.Account Delta
The account delta is impacted by the layout change, namely that non-fungible asset's were previously committed to by hashing their value. Because the value doesn't contain the faucet ID anymore, the delta also doesn't commit to the asset's issuer anymore and so the delta format needed to be changed to commit to the faucet ID. This means the asset key of the non-fungible asset is now included.
For security, the faucet ID prefix (specifically the bit in the account ID that determines the faucet type) must be at the same position in the layout to avoid introducing an ambiguity in the delta format. So, the layout of the fungible asset's delta elements was adapted as well. Overall it is now:
Fungible Assets
[domain = 1, was_added, faucet_id_suffix, faucet_id_prefix][amount_delta, 0, 0, 0]Non-Fungible Assets
[domain = 1, was_added, faucet_id_suffix, faucet_id_prefix][hash0, hash1, hash2, hash3](= ASSET_VALUE)See docs of
AccountDelta::to_commitmentfor all the details.This change included updates to the delta commitment computation as well as how non-fungible assets are tracked in the delta.
Note that the
NonFungibleAssetDelta's internal representation is a bit ugly, but a larger refactors doesn't make sense at this point since this may change more fundamentally when generalizing assets.Asset Key Ordering
AssetVaultKeyimplementsOrdandPartialOrdbased onLexicographicWordfor use in the delta. Not strictly necessary, but convenient. I also added implementations forAssetthat delegate toAssetVaultKey, but these are currently unused.Asset Modules
This PR commits further to the
fungible_assetand nownon_fungible_assetmodule in the kernel for implementing their respective logic. If we move this logic elsewhere later, this should make this process easier.Transaction Outputs
Since the fee asset is a fungible asset, and it is represented as a double word, the
FEE_ASSET_VALUEtx output needed to be adapted, as it would now only contain the amount, but not the faucet ID. The new approach is to output the individual required elements from the fungible asset, i.e. its faucet ID limbs and the amount. This also happens to make room for a full word on the output stack.Tests
Fixes a couple of tests where asset key and value were incorrectly ordered but the test didn't fail before, presumably due to the similar layout of key and value.
Migration
miden::protocol::active_account::has_non_fungible_assetnow takes anASSET_KEYas an input.miden::protocol::asset::create_non_fungible_assetnow takes the full account ID as an input.to_key_wordandto_value_wordmethods, andfrom_key_valueandfrom_key_value_wordsconstructors.NonFungibleAssetDelta::new's signature has changed.FungibleAsset::newandAsset::is_(non_)fungibleare no longerconst(I'm not sure these can realistically be used in aconstcontext and it restricts future non-breaking internal changes).{Asset, NonFungibleAsset, FungibleAsset}::faucet_id_prefixwas removed.NonFungibleAsset's andNonFungibleAssetDetails' structure has changed. They both contain the fullAccountIdnow.AssetVaultKey::{new_unchecked, faucet_id_prefix, as_word}were removed. Usenew,faucet_idandto_wordinstead.AssetVaultKey::from_account_idrenamed tonew_fungiblesince non-fungible asset vault keys cannot be created from just a faucet ID.Follow-Up
AssetIdnow is its own thing.Debugimplementation which is not a stable representation (could change with non-breaking changes) and it also shouldn't be used for error messages.closes #2328