ElementsProject / ELIPs

Elements Improvement proposals
12 stars 10 forks source link

Elip100 fixes #7

Closed RCasatta closed 11 months ago

RCasatta commented 11 months ago

Proposed fixes found while doing PR for rust-elements https://github.com/ElementsProject/rust-elements/pull/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

RCasatta commented 11 months ago

@miketlk @apoelstra

miketlk commented 11 months ago

@RCasatta, thank you for your input and suggestions. However, I'm not sure regarding the need to reverse transaction ID in the asset metadata. The current implementation in the Ledger Nano S app expects the same byte order of prevoutTxid as in JSON from the asset registry.

For example, let's look how USDt asset is listed in machine-oriented JSON and human-oriented web page, both from the Blockstream's registry (which I'm considering as a reference): USDt JSON USDt web page

In both sources, prevoutTxid is provided with the same byte order: 9596d259270ef5bac0020435e6d859aea633409483ba64e232b8ba04ce288668

The byte order of prevoutTxid is already reversed, indeed. When the hardware wallet app computes the asset entropy it hashes prevoutTxid in reverse order. That was done after studying implementation in Elements core.

RCasatta commented 11 months ago

Let's take the issuance you are referring to as example, which is tx abb4080d91849e933ee2ed65da6b436f7c385cf363fb4aa08399f1e27c58ff3d If you take the tx hex you can't find the hex string 9596d259270ef5bac0020435e6d859aea633409483ba64e232b8ba04ce288668 which is the prevoutTxid as you said and as shown in the explorer

While if you take the current elip 100 and search for the given prevout txid 3514a07cf4812272c24a898c482f587a51126beef8c9b76a9e30bf41b0cbe53c you can find it in the asset metadata hex. Thus the convention used is not the same.

The byte order of prevoutTxid is already reversed, indeed. When the hardware wallet app computes the asset entropy it hashes prevoutTxid in reverse order. That was done after studying implementation in Elements core.

Indeed the asset tag result coherent with the txid also in the rust-elements impl

That said, we can decide to remove the commit, and just mention to encode txid in reverse with respect to the consensus encoding in the <compact size uint contractLen><contract><32-byte prevoutTxid><32-bit little endian uint prevoutIndex> format, especially I understand if there is code in production, but in my opinion it would be better to change if possible.

miketlk commented 11 months ago

I like your idea to align asset metadata with network transaction format. But in my opinion, it's not the right scope since the asset metadata is neither a kind of transaction nor intended to be used as a drop-in piece to compose a transaction.

To me, it makes more sense to consider it a direct representation of an asset JSON taken from the registry, but in a more compact format and pruned to make processing easier on resource-limited platforms like hardware wallets. Initially, I thought about putting that JSON in PSET "as is" but realized that processing it would be hardly feasible on Nano S platform, which has severe memory limitations.

Unfortunately, I didn't find a specification of asset JSON format at the time of writing this ELIP100, to reference it there. But you are absolutely right that it's worth describing fields more precisely. So, I propose adding a notice that prevoutTxid is provided in reverse byte order, like in the asset registry's JSON. Would it work for your application?

miketlk commented 11 months ago

An additional benefit of using JSON-alike format of fields is that it makes unit and integration tests more human-readable, as there is a direct match between test data and what is seen in assets' JSON or web pages.

miketlk commented 11 months ago

But I understand that having transaction identifiers in different byte orders within a single PSET is a potential source of misunderstanding. So it's probably worth changing the implementation and specs to avoid it.

RCasatta commented 11 months ago

I still think it's better to change the test vector, but I am fine if we want to remove 86370e354dfd45a6c9c3c2659a3fc102657d18d3 and instead add a comment about the bytes order.

I leave this decision to you @miketlk and I will update the PR accordingly

miketlk commented 11 months ago

I leave this decision to you @miketlk and I will update the PR accordingly

I vote for merging your PR, as for me, the clarity and consistency of PSET specs outweigh the need to change the code.

@RCasatta @apoelstra