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

Add "Getting Started" documentation for tokens #306

Merged
merged 5 commits into from
Nov 24, 2021

Conversation

awrichar
Copy link
Contributor

@awrichar awrichar commented Oct 28, 2021

Initial docs for #218.

Some of the routes described here are still being updated by @eberger727 in #307, so should wait for that PR before merging this.

I've published the docs in this PR to my own FireFly fork here, if you like reading the pretty pages instead of the Markdown: https://awrichar.github.io/firefly/gettingstarted/mint_tokens.html

@codecov-commenter
Copy link

codecov-commenter commented Oct 28, 2021

Codecov Report

Merging #306 (7edd2a2) into main (ce74fe3) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##              main      #306    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files          224       234    +10     
  Lines        12387     12788   +401     
==========================================
+ Hits         12387     12788   +401     
Impacted Files Coverage Δ
pkg/fftypes/batch.go 100.00% <0.00%> (ø)
pkg/fftypes/event.go 100.00% <0.00%> (ø)
pkg/fftypes/bigint.go 100.00% <0.00%> (ø)
pkg/database/filter.go 100.00% <0.00%> (ø)
pkg/fftypes/message.go 100.00% <0.00%> (ø)
pkg/fftypes/tokenpool.go 100.00% <0.00%> (ø)
internal/config/config.go 100.00% <0.00%> (ø)
internal/assets/manager.go 100.00% <0.00%> (ø)
internal/apiserver/server.go 100.00% <0.00%> (ø)
internal/assets/token_pool.go 100.00% <0.00%> (ø)
... and 51 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ce74fe3...7edd2a2. Read the comment docs.

Copy link
Contributor

@peterbroadhurst peterbroadhurst left a comment

Choose a reason for hiding this comment

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

Really clear and straight forwards thanks @awrichar

Couple of comments, and deferral to @jimthematrix for a final review on this one before it goes in.

- FireFly provides an abstraction layer for multiple types of tokens
- Tokens are grouped into _pools_, which each represent a particular type or class of token
Copy link
Contributor

Choose a reason for hiding this comment

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

Wonder if an example worthwhile here, such as?

Examples of how a pool might be used by a given token connector implementation:
- ERC-721: A contract instance, ready for issuance of NFTs within unique identities in the pool
- ERC-20 / ERC-777: A token contract instance, with a single fungible pool of value - "a coin"
- ERC-1155: An efficient to allocate isolated pool of fungible or non-fungible tokens (see ref implementation)
- ERC-1400 (and related security token standards): An partition within an overall fungible pool, representing part of the value of an overall non-fungible asset

Could do with @jimthematrix checking on the terminology above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could be, and maybe not limited to pools... Maybe we need an extra section of "Example uses" to lay out a few examples of how you could map common token standards/use cases onto FireFly.

Copy link
Contributor

Choose a reason for hiding this comment

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

agree with the suggestion to add some details how a pool is mapped to the various contract specs.

agree with the suggested mapping above to ERC721, ERC20/777 and ERC1155.

on ERC-1400, however, a pool should be mapped to a contract instance instead of a partition. in ERC-1410 which describes the spec for partially fungible tokens, the interface methods that are compatible with ERC-20/ERC-777 all operate on the sum of all partitions. For example the balanceOf() method, same for totalSupply(): (ethereum/EIPs#1410):

balanceOf

Aggregates a token holders balances across all partitions. Equivalent to balanceOf in the ERC-20/777 specification.

MUST count the sum of all partition balances assigned to a token holder.

function balanceOf(address _tokenHolder) external view returns (uint256);

In addition, a token transfer is allowed to be received into a partition that is different than the partition it's sent from:

transferByPartition

When transferring tokens from a particular partition, it is useful to know on-chain (i.e. not just via an event being fired) the destination partition of those tokens. The destination partition will be determined by the implementation of this function and will vary depending on use-case.

_The function MUST return the bytes32 partition of the receiver.

function transferByPartition(bytes32 _partition, address _to, uint256 _value, bytes _data) external returns (bytes32)

Given the above interface in ERC-1410 (main spec of the security token standard captured by ERC-1400 as a compendium), the operations that target the sum of all partitions (eg balanceOf(), totalSupply(), authorizeOperator(), etc.) are already supported by the current tokens API interface when a pool is mapped to the contract instance, and it'd be straightforward to add support for partition based operations (eg balanceOfByPartition(), transferByPartition(), authorizeOperatorByPartition()) by simply adding additional payload parameters to indicate target partitions to the existing API endpoints.

On the other hand, if pools are mapped to partitions, then we have to add support for cross-pool transfers and aggregates in order to properly support the ERC-1410 operations.

Copy link
Contributor

Choose a reason for hiding this comment

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

if we agree with the above, we should also add that the current API endpoints support the ERC20, ERC777, ERC1155 and ERC721 contracts, while future work is needed to support ERC1400, in particular to support partitions (metadata to fungible tokens) within pools.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a new section with some of these examples.

docs/gettingstarted/mint_tokens.md Show resolved Hide resolved
docs/gettingstarted/mint_tokens.md Show resolved Hide resolved
@awrichar
Copy link
Contributor Author

Should I add a section on the FireFly configuration file? ie show the sample tokens config that is generated by the CLI?

Copy link
Contributor

@jimthematrix jimthematrix left a comment

Choose a reason for hiding this comment

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

Nice writeup. Added my opinion on support for ERC-1400, and other relatively minor comments. Happy to discuss more if there are different thoughts.

- FireFly provides an abstraction layer for multiple types of tokens
- Tokens are grouped into _pools_, which each represent a particular type or class of token
Copy link
Contributor

Choose a reason for hiding this comment

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

agree with the suggestion to add some details how a pool is mapped to the various contract specs.

agree with the suggested mapping above to ERC721, ERC20/777 and ERC1155.

on ERC-1400, however, a pool should be mapped to a contract instance instead of a partition. in ERC-1410 which describes the spec for partially fungible tokens, the interface methods that are compatible with ERC-20/ERC-777 all operate on the sum of all partitions. For example the balanceOf() method, same for totalSupply(): (ethereum/EIPs#1410):

balanceOf

Aggregates a token holders balances across all partitions. Equivalent to balanceOf in the ERC-20/777 specification.

MUST count the sum of all partition balances assigned to a token holder.

function balanceOf(address _tokenHolder) external view returns (uint256);

In addition, a token transfer is allowed to be received into a partition that is different than the partition it's sent from:

transferByPartition

When transferring tokens from a particular partition, it is useful to know on-chain (i.e. not just via an event being fired) the destination partition of those tokens. The destination partition will be determined by the implementation of this function and will vary depending on use-case.

_The function MUST return the bytes32 partition of the receiver.

function transferByPartition(bytes32 _partition, address _to, uint256 _value, bytes _data) external returns (bytes32)

Given the above interface in ERC-1410 (main spec of the security token standard captured by ERC-1400 as a compendium), the operations that target the sum of all partitions (eg balanceOf(), totalSupply(), authorizeOperator(), etc.) are already supported by the current tokens API interface when a pool is mapped to the contract instance, and it'd be straightforward to add support for partition based operations (eg balanceOfByPartition(), transferByPartition(), authorizeOperatorByPartition()) by simply adding additional payload parameters to indicate target partitions to the existing API endpoints.

On the other hand, if pools are mapped to partitions, then we have to add support for cross-pool transfers and aggregates in order to properly support the ERC-1410 operations.

- FireFly provides an abstraction layer for multiple types of tokens
- Tokens are grouped into _pools_, which each represent a particular type or class of token
Copy link
Contributor

Choose a reason for hiding this comment

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

if we agree with the above, we should also add that the current API endpoints support the ERC20, ERC777, ERC1155 and ERC721 contracts, while future work is needed to support ERC1400, in particular to support partitions (metadata to fungible tokens) within pools.

- Each pool is classified as _fungible_ or _non-fungible_
- In the case of _non-fungible_ tokens, the pool is subdivided into individual tokens with a unique _token index_
- Within a pool, you may _mint_, _transfer_, and _burn_ tokens
Copy link
Contributor

Choose a reason for hiding this comment

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

suggest adding operational aliases (issue vs. mint, redeem vs. burn) used in different token specs:

Within a pool, you may mint (issue), transfer, and burn (redeem) tokens

docs/gettingstarted/mint_tokens.md Show resolved Hide resolved
```

By default, a broadcast message is used. In order to send a private message, specify `"type": "transfer_private"` in the message header,
Copy link
Contributor

Choose a reason for hiding this comment

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

a sample payload for a private message would be useful here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

## Sending data with a transfer

All transfers (as well as mint/burn operations) support an optional `message` parameter that contains a broadcast or private
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we effectively hijacked the _data field of the various operations that may have been needed for further on-chain authorizations (e.g signatures by parties other than the sender to indicate approval). as a mental note we should consider supporting an additional payload parameter to include into the _data field as-is

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jimthematrix yes, you're correct. The token connector exposes a data field, and FireFly packs JSON into this field to identify messages and other metadata. But FireFly itself does not (currently) expose any way for the user to control or add to this field from the level of the FireFly APIs.

only the creator of a pool is allowed to mint - but each connector may define its own permission model.

`POST` `/api/v1/namespaces/default/tokens/mint`
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the token pool name part is missing from this path

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay actually I see now that it just works if there's only one pool defined. Does this URL break if there is more than one pool?

Copy link
Contributor Author

@awrichar awrichar Nov 24, 2021

Choose a reason for hiding this comment

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

The pool name is not in the new paths (if there is only one pool, the name is optional; if there is more than one pool, you pass it in the body). The old path with the pool name is deprecated.

Copy link
Contributor

@nguyer nguyer left a comment

Choose a reason for hiding this comment

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

Looks great. I ran through the instructions in Postman and it worked for me too. Jim has requested changes, but if he dismisses or approves after updates, I'm fine with this being merged. Thanks!

@awrichar awrichar merged commit 0646451 into hyperledger:main Nov 24, 2021
@awrichar awrichar deleted the tokendocs branch November 24, 2021 17:22
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