Skip to content

Single-prefix+bitfield token encoding #33

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

Closed

Conversation

A60AB5450353F40E
Copy link
Contributor

Implement #28

Also found some nits in introspection opcode definitions and test vectors

@bitjson
Copy link
Member

bitjson commented Jul 18, 2022

Thanks for the PR @A60AB5450353F40E! Still working on reviewing (by implementing in Libauth and updating those test vectors) – should have solid thoughts here in a few days. 👍

readme.md Outdated
4. `commitment` – If `commitment_length` is non-zero, a **token commitment** byte string of `commitment_length` is required.
5. `ft_amount` – a **token amount** (encoded as in `CompactSize` format) with a minimum value of `0` (`0x00`) and a maximum value equal to the maximum VM number, `9223372036854775807` (`0xffffffffffffff7f`).
2. `<token_format | nft_capability>` - A byte encoding two half-byte (4-bit) fields
1. `<token_format> - A bitfield indicating the token payload that follows, defined at the higher half of the serialized byte, to be deserialized using `token_format = serialized_byte & 0xf0`.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit-pick. Serialization is to turn something into bytes so using serialized byte is non-sensical. Its just a byte. Bitfield is indeed the best name that all programmers should understand.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How's this: 10707fe

Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM

readme.md Outdated
3. `0x50` (`b01010000`) - Both fungible tokens and a non-fungible token
4. `0x60` (`b01100000`) - Non-fungible token with a commitment
5. `0x70` (`b01110000`) - Both fungible tokens and a non-fungible with a commitment
2. `<nft_capability>` – A byte indicating the capability of a non-fungible token, defined at the lower half of the serialized byte, to be deserialized using `token_format = serialized_byte & 0x0f`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Starting with "A byte" is confusing as you are simply continuing the bitfield from above.

Or, in other words, you are listing the bit-field fields and you are at the same time listing the way it is encoded and that is confusing.

I suggest simply stating it is a bitfield and listing its items. This allows you to list less (for instance commitment is a single bit, but you list multiple combinations above, actually making it more confusing).

For an example, check the docs on https://doc.qt.io/qt-6/qflags.html#details specifically the 'example' which simply lists each bitfield.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I liked Jason's style of listing only allowed combinations (we don't allow no tokens, or a commitment without an NFT) so thought to keep it consistent with old ver.

Copy link
Contributor

Choose a reason for hiding this comment

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

The allowed combinations is listed a couple of paragraphs below, though.

In a technical documentation is helps to just stick to the technical docs without fluff. These are flags in a bitfield and each can be documented individually.
That way any technical person can read it faster and without fault and doesn't have to read the fluffy descriptions in order to find out that there is an undocumented "commitment" flag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How's this: deb25d6

Copy link
Contributor

Choose a reason for hiding this comment

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

looks great!

byte -> bitfield, see cashtokens#33 (review)
A60AB5450353F40E added a commit to A60AB5450353F40E/cashtokens that referenced this pull request Jul 19, 2022
@bitjson
Copy link
Member

bitjson commented Jul 20, 2022

@A60AB5450353F40E do you consider the ordering important for bits "has non-fungible token" (0b01000000) and "has commitment length" (0b00100000)? Would it matter if we swapped those positions?

@A60AB5450353F40E
Copy link
Contributor Author

A60AB5450353F40E commented Jul 20, 2022

@bitjson not important, it just made sense to put them in the same order the related fields will appear.

@bitjson
Copy link
Member

bitjson commented Jul 28, 2022

Merged in e61ee76, thanks!

@bitjson bitjson closed this Jul 28, 2022
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