Skip to content

Simplify compute_asset_desc_hash function#161

Merged
ConstanceBeguier merged 6 commits intozsa1from
clean_asset_desc
May 6, 2025
Merged

Simplify compute_asset_desc_hash function#161
ConstanceBeguier merged 6 commits intozsa1from
clean_asset_desc

Conversation

@ConstanceBeguier
Copy link
Collaborator

@ConstanceBeguier ConstanceBeguier commented Apr 25, 2025

Update compute_asset_desc_hash function

  • Now accepts &NonEmpty<u8> instead of &[u8]
  • Returns [u8; 32] directly (no longer wrapped in Result)
  • Panics if asset_desc is not valid UTF-8

Update get_burn_tuple function

  • Now takes asset_desc_hash as input instead of asset_desc
  • Changes the function’s visibility to private

Cleanup unused code

  • Removed unused errors: WrongAssetDescSize, InvalidAssetDescHashLength
  • Removed unused constant: MAX_ASSET_DESCRIPTION_SIZE
  • Removed unused function: is_asset_desc_of_valid_size

@QED-it QED-it deleted a comment from what-the-diff bot Apr 25, 2025
@PaulLaux PaulLaux requested review from PaulLaux and Copilot April 30, 2025 09:54
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR simplifies the compute_asset_desc_hash function in accordance with the new ZIP227 spec by requiring nonempty asset descriptions and removing now-unneeded error variants.

  • Converts asset_desc from a slice to a NonEmpty type in several modules.
  • Eliminates error handling for invalid sizes by using NonEmpty::from_slice with an expect message.
  • Removes the is_asset_desc_of_valid_size helper and related error variants.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
tests/zsa.rs Updated compute_asset_desc_hash call to use NonEmpty rather than unwrap a Result.
tests/issuance_global_state.rs Adjusted multiple compute_asset_desc_hash calls to adopt the NonEmpty approach.
src/note/asset_base.rs Modified random asset generation to use NonEmpty and removed the unused asset_desc size check.
src/issuance.rs Simplified compute_asset_desc_hash; updated tests and removed error propagation.
src/bundle/burn_validation.rs Updated compute_asset_desc_hash invocation to use NonEmpty.

@ConstanceBeguier
Copy link
Collaborator Author

I just rebased on zsa1 branch

Copy link
Collaborator

@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.

added some comments for extra consistency. Also:

  • please fix issue_bundle_asset_desc_roundtrip() I'm not sure what it checks now but it is surly not seet_desc roundtrip. Let's simplify it to check one asset_desc roundtrip. The end of the test should compute the hash again and check that we can get it via get_action_by_desc_hash()

Separate " // Not well-formed as per Unicode 15.0 specification, Section 3.9, D92" to a different test. We want to fail on non-unicode strings. right?

@ConstanceBeguier
Copy link
Collaborator Author

ConstanceBeguier commented May 5, 2025

image

It seems that the check on UTF-8 encoding is not done in Orchard.

I updated the issue_bundle_asset_desc_roundtrip test to remove not well-formed UTF-8

Copy link
Collaborator

@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.

approved with minor comments

get_burn_tuple(b"Asset 1", 20),
get_burn_tuple(b"Asset 3", 10),
get_burn_tuple(
&compute_asset_desc_hash(&NonEmpty::from_slice(b"Asset 1").unwrap()),
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we add a From<> trait for NonEmpty in our code so we can do b"Asset 1".into() ?

Copy link
Collaborator Author

@ConstanceBeguier ConstanceBeguier May 6, 2025

Choose a reason for hiding this comment

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

I think it is not possible because neither From nor NonEmpty are defined in the current crate.

@ConstanceBeguier ConstanceBeguier merged commit 9a25bd1 into zsa1 May 6, 2025
17 checks passed
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.

3 participants