Skip to content

ZIP 227: Replace asset_desc with its hash in AssetId#975

Merged
nuttycom merged 3 commits into
mainfrom
zip227-comm-asset-desc
Feb 25, 2025
Merged

ZIP 227: Replace asset_desc with its hash in AssetId#975
nuttycom merged 3 commits into
mainfrom
zip227-comm-asset-desc

Conversation

@str4d
Copy link
Copy Markdown
Collaborator

@str4d str4d commented Feb 6, 2025

No description provided.

Comment thread zips/zip-0227.rst
.. math:: \mathsf{ZSAValueBase}(\mathsf{AssetDigest_{AssetId}}) := \mathsf{GroupHash}^\mathbb{P}(\texttt{"z.cash:OrchardZSA"}, \mathsf{AssetDigest_{AssetId}})

where $\mathsf{GroupHash}^\mathbb{P}$ is defined as in [#protocol-concretegrouphashpallasandvesta]_.
This Asset Base is included in shielded notes within the shielded protocol.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
This Asset Base is included in shielded notes within the shielded protocol.
This Asset Base is included in shielded notes within the shielded protocol.
The definition of $\mathsf{ZSAValueBase}$ for the OrchardZSA protocol is given
in `OrchardZSA Custom Assets`_.

Comment thread zips/zip-0227.rst
the issuance. $\mathsf{asset\_desc}$ is a non-empty byte sequence of at most 512 bytes
which SHOULD be a well-formed UTF-8 code unit sequence according to Unicode 15.0.0 or later.
the issuance. $\mathsf{asset\_desc}$ is a non-empty byte sequence which SHOULD be a
well-formed UTF-8 code unit sequence according to Unicode 15.0.0 or later.
Copy link
Copy Markdown
Collaborator

@daira daira Feb 6, 2025

Choose a reason for hiding this comment

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

Suggested change
well-formed UTF-8 code unit sequence according to Unicode 15.0.0 or later.
well-formed UTF-8 code unit sequence according to Unicode 15.0.0 or later. [#Unicode]_

Also add in References:

.. [#Unicode] `Unicode 15.0.0. Unicode Consortium, September 2022. <https://www.unicode.org/versions/Unicode15.0.0/>`_

Comment thread zips/zip-0227.rst
- The issuance key structure is independent of the original key tree, but derived in an analogous manner (via ZIP 32). This keeps the issuance details and the Asset Identifiers consistent across multiple shielded pools. It also separates the issuance authority from the spend authority, allowing for the potential transfer of issuance authority without compromising the spend authority.
- The Custom Asset is described via a combination of the issuance validating key and an asset description string, to preclude the possibility of two different issuers creating colliding Custom Assets.
- The $\mathsf{asset\_desc}$ is a general byte string in order to allow for a wide range of information type to be included that may be associated with the Assets. Some are:
- We require non-zero fees in the presence of an issue bundle, in order to preclude the possibility of a transaction containing only an issue bundle. If a transaction includes only an issue bundle, the SIGHASH transaction hash would be computed solely based on the issue bundle. A duplicate bundle would have the same SIGHASH transaction hash, potentially allowing for a replay attack.
Copy link
Copy Markdown
Collaborator

@daira daira Feb 6, 2025

Choose a reason for hiding this comment

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

TODO(daira) file a separate issue: this is no longer needed as a separate rule, since we require at least one action from the same shielded protocol as the notes being issued.

Comment thread zips/zip-0227.rst

- If issuance transactions include the asset descriptions directly, wallets will discover
them during scanning. This is an "attractive nuisance" because it would result in
wallets being more likely to expose the asset description directly to users without any
Copy link
Copy Markdown
Collaborator

@daira daira Feb 6, 2025

Choose a reason for hiding this comment

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

Suggested change
wallets being more likely to expose the asset description directly to users without any
wallets being more likely to expose the asset description directly to users, without any

Comment thread zips/zip-0227.rst
verification that the received asset has the value that a user might expect from that
description. By instead using a collision-resistant hash of an asset description,
wallets are forced to look up the corresponding asset description when a payment is
received in an unknown asset. That lookup can be mediated by a trusted party or common
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
received in an unknown asset. That lookup can be mediated by a trusted party or common
received in an unknown asset. That lookup can be mediated by a trusted party or a common

Copy link
Copy Markdown
Collaborator

@daira daira left a comment

Choose a reason for hiding this comment

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

utACK with suggestions

Copy link
Copy Markdown
Contributor

@PaulLaux PaulLaux left a comment

Choose a reason for hiding this comment

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

Yes, technically, this change is fully possible.

However, if we don't post the asset_desc to the blockchain at least once, we are burdening the issuer and users with keeping and maintaining a mapping from asset_desc to assetDescHash.

If this mapping is lost, it is not recoverable.

This will increase the burden and increase the operation cost of the issuer. Is this what we really want?

The alternative is to post it to the chain once and collect a proper fee for the storage.

@arya2
Copy link
Copy Markdown
Collaborator

arya2 commented Feb 12, 2025

The alternative is to post it to the chain once and collect a proper fee for the storage.

Including an asset description in the first issuance but only an asset digest in later issuances seems practically similar to and better than including the asset description in a memo by convention.

@nuttycom
Copy link
Copy Markdown
Contributor

nuttycom commented Feb 25, 2025

@PaulLaux The purpose is not to save blockchain space. The purpose is to avoid the kind of spoofing and/or malicious-URL-in-cleartext problems that are rampant in the Ethereum ecosystem with ERC-20s. From my perspective, it's an absolute non-starter to have this in cleartext on the chain, given that it is not authenticatable, as we discussed in the rationale section. The central point is that wallets MUST NOT be able to naively display the asset description to a user.

I personally don't think that the asset description should be on-chain at all, but there's no way to stop an issuer from including it in a memo that is decryptable using a well-known key. If an issuer chooses to use a well-known ZIP 231 memo convention for this, then at least the asset description is prunable if it's determined that it is abusive (doxxing data, etc.)

@str4d
Copy link
Copy Markdown
Collaborator Author

str4d commented Feb 25, 2025

Per discussion in the zips channel of the R&D Discord, the approach we're going with is this PR + issuers optionally choosing to store asset_desc in the memo bundle for the first issuance transaction (as a convenience; wallets won't assume it is present).

@nuttycom nuttycom merged commit 0fb1511 into main Feb 25, 2025
@nuttycom nuttycom deleted the zip227-comm-asset-desc branch February 25, 2025 23:55
str4d added a commit that referenced this pull request Feb 26, 2025
vivek-arte added a commit to QED-it/librustzcash that referenced this pull request Apr 23, 2025
This adds the changes needed to support the move to using the hash of
the Asset Description string instead of the entire string, as initially
changed in zcash/zips#975.

The test vectors are also updated from QED-it/zcash-test-vectors#28

---------

Co-authored-by: alexeykoren <2365507+alexeykoren@users.noreply.github.com>
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.

5 participants