Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement Serializable for FungibleAsset #907

Merged
merged 3 commits into from
Oct 14, 2024

Conversation

PhilippGackstatter
Copy link
Contributor

Implements Serializable for FungibleAsset.

I reconsidered whether it would make sense to

  • forward the Serializable implementation from Asset to FungibleAsset, but Asset has a dedicated byte conversion implementation (impl From<Asset> for [u8; Asset::SERIALIZED_SIZE]) which makes more sense to keep using since it means that serialization is only defined in one place.
  • forward the serialization and size hint implementation from FungibleAssetDelta to FungibleAsset but this is a special case. In the delta implementation we serialize the asset as 16 bytes whereas elsewhere it is serialized as 32 bytes.

The Serializable implementation reuses the 32-byte asset format even though it could be made more compact at 16 bytes (omitting the zeroes).

I guess the argument for keeping all assets at one word for now is that there is no need for a discriminant in the serialization format for Asset since assets are already designed to be distinguishable by their structure plus it is much easier to handle assets in the VM when they are all the same length. Such a discriminant would only cost 1 byte per asset though and the saving per fungible asset would be 15 bytes, so it seems worthwhile to think about that optimization for use cases outside the VM like network transmission of objects containing assets. As far as I can tell we could come up with different formats for assets within the VM (e.g. one word for its easy handling) and outside the VM (e.g. with discriminant and space-optimized for storage or transmission purposes).

Wouldn't it already be possible to make assets distinguishable by their first or second byte such that no discriminant was needed and we could still store fungible assets as 16 bytes? For instance, the serialized format for a fungible asset could be: [amount, faucet_id] instead of [amount, 0, 0, faucet_id]. Assets are thus always at least 2 Felts long. So to distinguish assets we could always inspect the second Felt and determine the asset's type from the faucet's type. This could use up to 50% less space (if only fungible assets are used) but it might also be a premature optimization.
I might easily be missing some context though, just wanted to raise this point I found interesting :)

This is also somewhat interesting in the context of #140 which might lead to a rework of the asset format. If we determine that the amount of a fungible asset suffices to be 64-192 bits, meaning we can keep them at one word, and we have to go to two words for non fungible assets, that space saving per fungible asset would go up even further.

@PhilippGackstatter PhilippGackstatter linked an issue Oct 7, 2024 that may be closed by this pull request
@PhilippGackstatter PhilippGackstatter force-pushed the pgackst-fungible-asset-serializable-900 branch from 0df18f7 to f557e7b Compare October 7, 2024 17:34
@bobbinth
Copy link
Contributor

bobbinth commented Oct 8, 2024

I guess the argument for keeping all assets at one word for now is that there is no need for a discriminant in the serialization format for Asset since assets are already designed to be distinguishable by their structure plus it is much easier to handle assets in the VM when they are all the same length. Such a discriminant would only cost 1 byte per asset though and the saving per fungible asset would be 15 bytes, so it seems worthwhile to think about that optimization for use cases outside the VM like network transmission of objects containing assets. As far as I can tell we could come up with different formats for assets within the VM (e.g. one word for its easy handling) and outside the VM (e.g. with discriminant and space-optimized for storage or transmission purposes).

Yes, I think this is a good idea. In the VM we can always represent assets uniformly, but outside of the VM we could treat the differently to save space.

One important thing though: when computing asset hash, we should use in-VM asset representation. So, both fungible and non-fungible assets should be reducible to words (single word now and potentially double-word in the future).

Wouldn't it already be possible to make assets distinguishable by their first or second byte such that no discriminant was needed and we could still store fungible assets as 16 bytes? For instance, the serialized format for a fungible asset could be: [amount, faucet_id] instead of [amount, 0, 0, faucet_id]. Assets are thus always at least 2 Felts long. So to distinguish assets we could always inspect the second Felt and determine the asset's type from the faucet's type. This could use up to 50% less space (if only fungible assets are used) but it might also be a premature optimization.

Yes, let's use the following serialization:

fungible asset: [faucet_id, amount],
non-fungible asset: [faucet_id, d0, d2, d3]

This way, we'd be able to determine the type of the asset from the first element and no extra discriminant would be needed. For non-fungible assets, we'd have to do a bit of reshuffling to arrange the elements in the correct order (i.e., [d0, faucet_id, d2, d3]), but I think that's OK.

Also, I'd probably get rid of the following conversion for FungibleAsset and probably Asset as well:

  • From<FungibleAsset> for [u8; 32]
  • TryFrom<[u8; 32]> for FungibleAsset

I'm actually not sure if we use them anywhere.

@PhilippGackstatter PhilippGackstatter force-pushed the pgackst-fungible-asset-serializable-900 branch from f557e7b to 80e71f5 Compare October 9, 2024 09:21
@PhilippGackstatter
Copy link
Contributor Author

let's use the following serialization:

fungible asset: [faucet_id, amount],
non-fungible asset: [faucet_id, d0, d2, d3]

Sounds great, did that now 👍

One important thing though: when computing asset hash, we should use in-VM asset representation.

I kept the From<Asset> for Word conversion which is used when computing the asset commitment so it's still using the in-VM (word) representation for that:

let asset_word: Word = (*asset).into();

Also removed unused conversion functions to bytes 👍

This PR is ready for review now.

@PhilippGackstatter PhilippGackstatter force-pushed the pgackst-fungible-asset-serializable-900 branch from 80e71f5 to 8ae9c73 Compare October 9, 2024 12:09
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thank you! I left one comment inline - after it is addressed, we can merge.

objects/src/assets/nonfungible.rs Outdated Show resolved Hide resolved
@PhilippGackstatter PhilippGackstatter force-pushed the pgackst-fungible-asset-serializable-900 branch from bcb4617 to 2ea5138 Compare October 14, 2024 08:40
@PhilippGackstatter PhilippGackstatter merged commit c0317c2 into next Oct 14, 2024
8 checks passed
@PhilippGackstatter PhilippGackstatter deleted the pgackst-fungible-asset-serializable-900 branch October 14, 2024 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement Serializable for FungibleAsset
2 participants