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

ELIP-0100 implementation #182

Merged
merged 3 commits into from
Nov 22, 2023
Merged

Conversation

RCasatta
Copy link
Collaborator

@RCasatta RCasatta commented Oct 27, 2023

@RCasatta RCasatta marked this pull request as ready for review November 13, 2023 13:09
@RCasatta
Copy link
Collaborator Author

Removed draft status since the proposed changes ElementsProject/ELIPs#7 (comment) have been agreed upon.

(Should wait for ElementsProject/ELIPs#7 merge regardless)

apoelstra added a commit to ElementsProject/ELIPs that referenced this pull request Nov 13, 2023
6abc856 elip-0100: Increase lenght of invalid ticker (Riccardo Casatta)
86370e3 elip-0100: fix asset metadata value test vector (Riccardo Casatta)

Pull request description:

  Proposed fixes found while doing PR for rust-elements ElementsProject/rust-elements#182

  In particular about the proposed change for the value, there is a test showing the changed bytes are only the ones involving the txid: https://github.com/ElementsProject/rust-elements/pull/182/files#diff-cde6cc19a998ab9ddf07c6afe978e4129b8d196bc347bcfe79caf2134fabd2a7R371

Top commit has no ACKs.

Tree-SHA512: 6d710cd7b808092d92cac9210fd238c8e92e0e3a42d485d3c2a7bc32b844cf0b67b869f48dc0480ec1403c3e250c308b0f958fe8acf65827365d52b307ff3188

self.contract
.as_bytes()
.to_vec()
Copy link
Member

Choose a reason for hiding this comment

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

In 2ec896f:

Tthis to_vec shouldn't be necessary -- I think we can directly consensus-encode byte slices.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I went lazy with vec for having the initial VarInt...
I see we have consensus_encode_with_size which is the right choice here, updated.

@apoelstra
Copy link
Member

2ec896f looks good other than the vec thing.

@RCasatta RCasatta force-pushed the elip100 branch 2 times, most recently from 8f73ec5 to 2d7402f Compare November 16, 2023 18:55
@apoelstra
Copy link
Member

cargo test --no-default-features is now broken. (I'm surprised that CI does not catch this, but my local CI does.)

as defined in https://github.com/ElementsProject/ELIPs/blob/main/elip-0100.mediawiki
but excluding contract validation since:
- it will go in a standalone elip
- requires other deps, in particular for domain validation
according to the help the option is deprecated and equivalent to `--workspace`
and we haven't a workspace.
Moreover, and most importantly, the option somehow activates features which
should not be present. for example:
`cargo tree --no-default-features -e no-dev --all | grep serde` has dependencies in the output while without `--all` returns nothing
@RCasatta
Copy link
Collaborator Author

RCasatta commented Nov 18, 2023

cargo test --no-default-features is now broken.

fixed

I'm surprised that CI does not catch this, but my local CI does.

The issue was with --all, see commit a8f4c8c description

@apoelstra
Copy link
Member

The issue was with --all, see commit a8f4c8c description

Lol, great catch. Annoying that cargo doesn't reject this.

Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK a8f4c8c

@apoelstra apoelstra merged commit a19e347 into ElementsProject:master Nov 22, 2023
5 checks passed
apoelstra added a commit that referenced this pull request Nov 23, 2023
add54c8 elip100: add missing AssetMetadata::new method (Riccardo Casatta)

Pull request description:

  Missing from #182  otherwise there is no way to instantiate `AssetMetadata` and call `add_asset_metadata`

ACKs for top commit:
  apoelstra:
    ACK add54c8

Tree-SHA512: 1a5e935aa76a0f288d8ac1c0917ccf441991f32b830bab26c9b1146a3d462bfeac1caad1c3035f2f54669731bef779ea72d2cd27ff9d65b90c38f0253e4274b1
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.

2 participants