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

added docs to describe asset ft behavior #351

Merged
merged 12 commits into from
Jan 26, 2023
Merged

Conversation

miladz68
Copy link
Contributor

@miladz68 miladz68 commented Jan 18, 2023

This change is Reviewable

vertex451
vertex451 previously approved these changes Jan 18, 2023
Copy link
Contributor

@vertex451 vertex451 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @dzmitryhil, @wojtek-coreum, and @ysv)

Copy link
Contributor

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 20 unresolved discussions (waiting on @miladz68, @wojtek-coreum, and @ysv)


-- commits line 2 at r1:
@ysv @miladz68 Guys, but what about the spec in the way we have it for the feemodel? Do we want to have the same later or this one is final?


x/asset/ft/spec/README.md line 2 at r1 (raw file):

# Overview
This module called `assetft` is responsible to provide tokenization features for the fungible token part, described in Coreum's whitepaper. There is also another module under development called `assetnft` which is responsible for the NFT part of asset tokenization. 

IMO, we don't need that reference described in Coreum's whitepaper. Same for the nft we don't need it to be mentioned it here.

Code quote:

described in Coreum's whitepaper

x/asset/ft/spec/README.md line 4 at r1 (raw file):

This module called `assetft` is responsible to provide tokenization features for the fungible token part, described in Coreum's whitepaper. There is also another module under development called `assetnft` which is responsible for the NFT part of asset tokenization. 

Here in this doc we will provide the detailed behavior of fungible tokens as they are implemented. Please note that at the time of writing this doc, the behavior described here is closer to the implementation than the whitepaper and if there are differences, the behavior described here should be considered correct. This doc will describe transaction types and their behavior. For the fields and their meaning used in transactions please consult the proto files in `coreum/proto/coreum/asset/ft`.

Why do we need to keep so many warnings, let's just describe how the module works without warnings and references.


x/asset/ft/spec/README.md line 6 at r1 (raw file):

Here in this doc we will provide the detailed behavior of fungible tokens as they are implemented. Please note that at the time of writing this doc, the behavior described here is closer to the implementation than the whitepaper and if there are differences, the behavior described here should be considered correct. This doc will describe transaction types and their behavior. For the fields and their meaning used in transactions please consult the proto files in `coreum/proto/coreum/asset/ft`.

Here is the list of the transactions provided by this module, we will discuss each of them separately. 

Let's replace the discuss to something else.

Code quote:

we will discuss

x/asset/ft/spec/README.md line 15 at r1 (raw file):

# Interaction with bank module, introducing wbank module
Since Coreum is based on Cosmos SDK, We should mention that Cosmos SDK provides the native bank module which is responsible to track fungible token creation and balances for each account. But this module does not allow anyone to create a fungible token, mint/burn it, and also does not allow for other features such as freezing and whitelisting. To work around this issue we have wrapped the `bank` module into the wbank module. 

You wrapped the bank into "", so probably the wbank` should be also wrapped.

Code quote:

wbank

x/asset/ft/spec/README.md line 17 at r1 (raw file):

Since Coreum is based on Cosmos SDK, We should mention that Cosmos SDK provides the native bank module which is responsible to track fungible token creation and balances for each account. But this module does not allow anyone to create a fungible token, mint/burn it, and also does not allow for other features such as freezing and whitelisting. To work around this issue we have wrapped the `bank` module into the wbank module. 

In `wbank` module we wrap the Send method of the `bank` module and intercept it with `BeforeSend` functions provided by `assetft` module. This allows `assetft` module to inject custom logic into bank's `Send` method and reject some transaction if whitelisting or freezing criteria is not met.

Not just Send


x/asset/ft/spec/README.md line 21 at r1 (raw file):

In a nutshell, `assetft` module interacts with `wbank` which in turn wraps the original `bank` module.

This structure allows to reuse the code provided by Cosmos SDK, and also reuse the infrastructure that the community provides (e.g explorers and wallets), but the downside is that the data regarding for each fungible token must be split and stored in 2 different places. It is apparent in the query structure. For example you want to query for frozen balances of a fungible token you need to query the `assetft` module but to get total supply, you must query the `bank` module. 

Let's not mention the downside. We should mention the downside only if the users must understand it, here is just an implementation detail.


x/asset/ft/spec/README.md line 30 at r1 (raw file):

All the information provided at the time of issuance is immutable and cannot be changed later.

#### Issuance fee

I would add the Issuance fee after the explanation of the issuance body, since it's and effect of the issuance.

Code quote:

Issuance fee

x/asset/ft/spec/README.md line 31 at r1 (raw file):

#### Issuance fee
Whenever a user wants to issue a fungible token, they have to pay some extra money as issuance fee, which is calculated on type of tx execution fee and will be burnt.

You need to add that it's managed by the gov.


x/asset/ft/spec/README.md line 35 at r1 (raw file):

#### Denom naming, Symbol and Precision
The way that denom is created is that the user provides a name for their subunit, and the Denom for the token, which is the main identifier of the token, will be created by appending the subunit with the issuer address separated with a dash (subunit-address). The user also provides the symbol and precision which will only be used for display purposes and will be stored in bank modules metadata field.

Why Denom is uppercase here?

Code quote:

Deno

x/asset/ft/spec/README.md line 37 at r1 (raw file):

The way that denom is created is that the user provides a name for their subunit, and the Denom for the token, which is the main identifier of the token, will be created by appending the subunit with the issuer address separated with a dash (subunit-address). The user also provides the symbol and precision which will only be used for display purposes and will be stored in bank modules metadata field.

For example to represent bitcoin on Coreum, once could choose satoshi as subunit, BTC as Symbol and 8 as precision. It means that if the issuer address is core1tr3w86yesnj8f290l6ve02cqhae8x4ze0nk0a8 then the denom will be `satoshi-devcore1tr3w86yesnj8f290l6ve02cqhae8x4ze0nk0a8` and since we have chosen BTC as symbol and 8 as precision, it will follow that (1 BTC = 10^8 `satoshi-devcore1tr3w86yesnj8f290l6ve02cqhae8x4ze0nk0a8`)

I would add the Issuance fee after the explanation of the issuance body, since it's and effect of the issuance.


x/asset/ft/spec/README.md line 39 at r1 (raw file):

For example to represent bitcoin on Coreum, once could choose satoshi as subunit, BTC as Symbol and 8 as precision. It means that if the issuer address is core1tr3w86yesnj8f290l6ve02cqhae8x4ze0nk0a8 then the denom will be `satoshi-devcore1tr3w86yesnj8f290l6ve02cqhae8x4ze0nk0a8` and since we have chosen BTC as symbol and 8 as precision, it will follow that (1 BTC = 10^8 `satoshi-devcore1tr3w86yesnj8f290l6ve02cqhae8x4ze0nk0a8`)

#### Burn Rate 

IMO the Burn Rate· and Send Commission Rate should be after the Features, but before the Issuance Fee


x/asset/ft/spec/README.md line 46 at r1 (raw file):

#### Token Features
When issuing a token, the issuer must decide which features are enabled on the token. For example if `minting` feature is enabled then it will allow the issuer to mint further tokens on top of the initial supply, otherwise no new tokens can be minted. Here is a list of all the features that can be enabled on the token. Each of these features affects how the token will behave and a detail description will be provided in the dedicated section for each feature.

must -> can Since you can issue without feature.

Code quote:

must

x/asset/ft/spec/README.md line 54 at r1 (raw file):

## Mint
If the minting feature is enabled, then issuer of the token can submit a Mint transaction to apply more tokens to the total supply. All the minted tokens will be transferred to the issuers account address.

Mint transaction to apply -> is the apply work correct here?

Code quote:

Mint transaction to apply

x/asset/ft/spec/README.md line 57 at r1 (raw file):

## Burn
The issuer of the token can burn the tokens that they hold. If the burning flag is enabled, then every holder of the token can burn the tokens they hold.

Why they?

Code quote:

they 

x/asset/ft/spec/README.md line 57 at r1 (raw file):

## Burn
The issuer of the token can burn the tokens that they hold. If the burning flag is enabled, then every holder of the token can burn the tokens they hold.

Flag or fearture?

Code quote:

flag

x/asset/ft/spec/README.md line 60 at r1 (raw file):

## Freeze/Unfreeze
If the freezing feature is enabled on a token, then the issuer of the token can freeze an account up to a limit for that token. The frozen amount can be more than what the user currently holds, an works even if the user holds zero. The user can only send the tokens that they hold in excess of the frozen amount. 

What does it mean?

Code quote:

up to a limit for that token

x/asset/ft/spec/README.md line 63 at r1 (raw file):

For example if the issuer freezes 1000 ABC tokens on account Y and this account holds 800, then they cannot move any of their tokens, but if the account receives 400 extra ABC tokens, their total balance will become 1200 and then can only spend 200 of it, since 1000 is Frozen.

Here is the description of behavior of the freezing feature:

It looks like the duplication of the previous paragraph.


x/asset/ft/spec/README.md line 74 at r1 (raw file):

- Frozen amount cannot be a negative value, it means that amount present in unfreeze transaction cannot be bigger than the current frozen amount

## Global Freeze 

What about unfreeze?


x/asset/ft/spec/README.md line 80 at r1 (raw file):

If the whitelisting feature is enabled, then every account that wishes to receive this token, must first be whitelisted by the issuer, otherwise they will not be able to receive that token. This feature allows the issuer to set whitelisted limit on any account, and then that account will be able to receive tokens only up to the whitelisted limit. If someone tries to send tokens to an account which will result in the whitelisted amount to be exceeded, the transaction will fail.

Here is the description of behavior of the whitelisting feature:

It looks like the duplication of the previous paragraph.

Copy link
Contributor

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 20 unresolved discussions (waiting on @miladz68, @wojtek-coreum, and @ysv)

Copy link
Contributor

@pgoos pgoos left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 23 unresolved discussions (waiting on @miladz68, @wojtek-coreum, and @ysv)


x/asset/ft/spec/README.md line 4 at r1 (raw file):

This module called `assetft` is responsible to provide tokenization features for the fungible token part, described in Coreum's whitepaper. There is also another module under development called `assetnft` which is responsible for the NFT part of asset tokenization. 

Here in this doc we will provide the detailed behavior of fungible tokens as they are implemented. Please note that at the time of writing this doc, the behavior described here is closer to the implementation than the whitepaper and if there are differences, the behavior described here should be considered correct. This doc will describe transaction types and their behavior. For the fields and their meaning used in transactions please consult the proto files in `coreum/proto/coreum/asset/ft`.

I don't like this sentence Please note that at the time of writing this doc, the behavior described here is closer to the implementation than the whitepaper and if there are differences, the behavior described here should be considered correct. Why do we need to have misalignments at all? The whitepaper should be updated as well.

Code quote:

Please note that at the time of writing this doc, the behavior described here is closer to the implementation than the whitepaper and if there are differences, the behavior described here should be considered correct.

x/asset/ft/spec/README.md line 15 at r1 (raw file):

# Interaction with bank module, introducing wbank module
Since Coreum is based on Cosmos SDK, We should mention that Cosmos SDK provides the native bank module which is responsible to track fungible token creation and balances for each account. But this module does not allow anyone to create a fungible token, mint/burn it, and also does not allow for other features such as freezing and whitelisting. To work around this issue we have wrapped the `bank` module into the wbank module. 

responsible to track should be responsible for tracking


x/asset/ft/spec/README.md line 21 at r1 (raw file):

In a nutshell, `assetft` module interacts with `wbank` which in turn wraps the original `bank` module.

This structure allows to reuse the code provided by Cosmos SDK, and also reuse the infrastructure that the community provides (e.g explorers and wallets), but the downside is that the data regarding for each fungible token must be split and stored in 2 different places. It is apparent in the query structure. For example you want to query for frozen balances of a fungible token you need to query the `assetft` module but to get total supply, you must query the `bank` module. 

regarding for should be just regarding
This sentence doesn't sound good For example you want to query for frozen balances of a fungible token you need to query the assetftmodule but to get total supply, you must query thebank module.
How about: For example, if you want to query for frozen balances of a fungible token, you need to query the assetft module but if you want to get the total supply, you must query the bank module.

Copy link
Contributor Author

@miladz68 miladz68 left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 22 unresolved discussions (waiting on @dzmitryhil, @pgoos, @wojtek-coreum, and @ysv)


-- commits line 2 at r1:

Previously, dzmitryhil (Dzmitry Hil) wrote…

@ysv @miladz68 Guys, but what about the spec in the way we have it for the feemodel? Do we want to have the same later or this one is final?

I argued that having it in that format brings no real value.


x/asset/ft/spec/README.md line 2 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

IMO, we don't need that reference described in Coreum's whitepaper. Same for the nft we don't need it to be mentioned it here.

The point of mentioning them is to describe the thought process on how we got here.


x/asset/ft/spec/README.md line 4 at r1 (raw file):

Previously, pgoos (Piotr Gesicki) wrote…

I don't like this sentence Please note that at the time of writing this doc, the behavior described here is closer to the implementation than the whitepaper and if there are differences, the behavior described here should be considered correct. Why do we need to have misalignments at all? The whitepaper should be updated as well.

Removed


x/asset/ft/spec/README.md line 4 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Why do we need to keep so many warnings, let's just describe how the module works without warnings and references.

Removed that sentence


x/asset/ft/spec/README.md line 6 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Let's replace the discuss to something else.

Done.


x/asset/ft/spec/README.md line 15 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

You wrapped the bank into "", so probably the wbank` should be also wrapped.

Done.


x/asset/ft/spec/README.md line 15 at r1 (raw file):

Previously, pgoos (Piotr Gesicki) wrote…

responsible to track should be responsible for tracking

Done, Great catch


x/asset/ft/spec/README.md line 17 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Not just Send

Done.


x/asset/ft/spec/README.md line 21 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Let's not mention the downside. We should mention the downside only if the users must understand it, here is just an implementation detail.

The doc is intended for developers mostly those that will join us, they need to understand it. I don't think we should structure this doc to be exposed to end users.


x/asset/ft/spec/README.md line 21 at r1 (raw file):

Previously, pgoos (Piotr Gesicki) wrote…

regarding for should be just regarding
This sentence doesn't sound good For example you want to query for frozen balances of a fungible token you need to query the assetftmodule but to get total supply, you must query thebank module.
How about: For example, if you want to query for frozen balances of a fungible token, you need to query the assetft module but if you want to get the total supply, you must query the bank module.

Done


x/asset/ft/spec/README.md line 30 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

I would add the Issuance fee after the explanation of the issuance body, since it's and effect of the issuance.

This is duplicate comment


x/asset/ft/spec/README.md line 31 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

You need to add that it's managed by the gov.

Done.


x/asset/ft/spec/README.md line 35 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Why Denom is uppercase here?

Fixed


x/asset/ft/spec/README.md line 37 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

I would add the Issuance fee after the explanation of the issuance body, since it's and effect of the issuance.

but the is no section explaining the request body. So I don't think they are related.


x/asset/ft/spec/README.md line 39 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

IMO the Burn Rate· and Send Commission Rate should be after the Features, but before the Issuance Fee

Done.


x/asset/ft/spec/README.md line 46 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

must -> can Since you can issue without feature.

but the decision must be made here. I mean "must" means the decision must be made, not the features must be enabled.


x/asset/ft/spec/README.md line 54 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Mint transaction to apply -> is the apply work correct here?

It is not a good word, changed to add


x/asset/ft/spec/README.md line 57 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Why they?

they means he/she.


x/asset/ft/spec/README.md line 57 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Flag or fearture?

Fixed


x/asset/ft/spec/README.md line 60 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

What does it mean?

Reworded


x/asset/ft/spec/README.md line 63 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

It looks like the duplication of the previous paragraph.

It contains the basic use case, which is duplicate but in shorter form, and also it contains edge cases which is not described.


x/asset/ft/spec/README.md line 74 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

What about unfreeze?

Added


x/asset/ft/spec/README.md line 80 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

It looks like the duplication of the previous paragraph.

It contains the basic use case, which is duplicate but in shorter form, and also it contains edge cases which is not described.

Copy link
Contributor

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 7 unresolved discussions (waiting on @miladz68, @pgoos, @silverspase, @wojtek-coreum, and @ysv)


x/asset/ft/spec/README.md line 2 at r1 (raw file):

Previously, miladz68 (milad) wrote…

The point of mentioning them is to describe the thought process on how we got here.

But that doc is the FT doc, it isn't the doc about all tokens we support.


x/asset/ft/spec/README.md line 21 at r1 (raw file):

Previously, miladz68 (milad) wrote…

The doc is intended for developers mostly those that will join us, they need to understand it. I don't think we should structure this doc to be exposed to end users.

But it isn't a downside, it's just an implementation detail. And we shouldn't pay attention to it.


x/asset/ft/spec/README.md line 63 at r1 (raw file):

Previously, miladz68 (milad) wrote…

It contains the basic use case, which is duplicate but in shorter form, and also it contains edge cases which is not described.

I still don't like the use cases, here. This is a technical doc, so the use cases should be somewhere outside in the coreum doc web-site for example.


x/asset/ft/spec/README.md line 80 at r1 (raw file):

Previously, miladz68 (milad) wrote…

It contains the basic use case, which is duplicate but in shorter form, and also it contains edge cases which is not described.

Same opinin as in the Freeze/Unfreeze

Copy link
Contributor

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @miladz68, @pgoos, @wojtek-coreum, and @ysv)

Copy link
Contributor Author

@miladz68 miladz68 left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @dzmitryhil, @pgoos, @wojtek-coreum, and @ysv)


x/asset/ft/spec/README.md line 2 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

But that doc is the FT doc, it isn't the doc about all tokens we support.

removed


x/asset/ft/spec/README.md line 21 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

But it isn't a downside, it's just an implementation detail. And we shouldn't pay attention to it.

Removed the word downside and I did some rewording, please take a look again. the fact that some part of information lives in bank module and some in asset ft module is not an implementation detail, but is part of the interface we expose to the outside world, I prefer to keep mentioning of this information here.


x/asset/ft/spec/README.md line 63 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

I still don't like the use cases, here. This is a technical doc, so the use cases should be somewhere outside in the coreum doc web-site for example.

This is the the expected behavior of the module. That is the main point of having this doc and the task.

Copy link
Contributor Author

@miladz68 miladz68 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 7 unresolved discussions (waiting on @dzmitryhil, @pgoos, @wojtek-coreum, and @ysv)


x/asset/ft/spec/README.md line 80 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Same opinin as in the Freeze/Unfreeze

lets dicsus there then.

Copy link
Contributor

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 5 unresolved discussions (waiting on @miladz68, @pgoos, @wojtek-coreum, and @ysv)


x/asset/ft/spec/README.md line 21 at r1 (raw file):

Previously, miladz68 (milad) wrote…

Removed the word downside and I did some rewording, please take a look again. the fact that some part of information lives in bank module and some in asset ft module is not an implementation detail, but is part of the interface we expose to the outside world, I prefer to keep mentioning of this information here.

Now better


x/asset/ft/spec/README.md line 63 at r1 (raw file):

Previously, miladz68 (milad) wrote…

This is the the expected behavior of the module. That is the main point of having this doc and the task.

@ysv ?


x/asset/ft/spec/README.md line 80 at r1 (raw file):

Previously, miladz68 (milad) wrote…

lets dicsus there then.

Sure

Copy link
Contributor

@pgoos pgoos left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 4 unresolved discussions (waiting on @dzmitryhil, @miladz68, @wojtek-coreum, and @ysv)


x/asset/ft/spec/README.md line 20 at r3 (raw file):

This structure allows to reuse the code provided by Cosmos SDK, and also reuse the infrastructure that the community provides (e.g explorers and wallets) It is apparent in the query structure. And it also leads to the fact that some of the information regarding fungible tokens will exists in the `assetft` module and some in the `bank` module. For example, if you want to query for frozen balances of a fungible token, you need to query the `assetft` module but if you want to get the total supply, you must query the bank module.

should be will exist

Code quote:

some of the information regarding fungible tokens will exists

Copy link
Contributor Author

@miladz68 miladz68 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 4 unresolved discussions (waiting on @dzmitryhil, @pgoos, @wojtek-coreum, and @ysv)


x/asset/ft/spec/README.md line 20 at r3 (raw file):

Previously, pgoos (Piotr Gesicki) wrote…

should be will exist

Done.

pgoos
pgoos previously approved these changes Jan 23, 2023
Copy link
Contributor

@pgoos pgoos left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @dzmitryhil, @miladz68, @wojtek-coreum, and @ysv)

@ysv ysv requested a review from dzmitryhil January 23, 2023 10:21
Copy link
Contributor

@ysv ysv left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 4 unresolved discussions (waiting on @dzmitryhil, @miladz68, and @wojtek-coreum)


x/asset/ft/spec/README.md line 63 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

@ysv ?

I like wording here and the way freezing is structured part by part in this section Here is the description of.
Maybe we should put For example if the issuer freezes 1000 ABC tokens... after this part but I'm not sure
mostly ok for me


x/asset/ft/spec/README.md line 70 at r1 (raw file):

- The user can only send their tokens in excess of the frozen amount.
- The user can receive tokens regardless of frozen limitation.
- The user can send their tokens to the issuer regardless of frozen limitation.

this one is not relevant as we decided recently please also check other parts where this mentioned
e.g global freeze

Copy link
Contributor

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @miladz68, @wojtek-coreum, and @ysv)


-- commits line 2 at r1:

Previously, miladz68 (milad) wrote…

I argued that having it in that format brings no real value.

@ysv?

Copy link
Contributor Author

@miladz68 miladz68 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @dzmitryhil, @wojtek-coreum, and @ysv)


x/asset/ft/spec/README.md line 70 at r1 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

this one is not relevant as we decided recently please also check other parts where this mentioned
e.g global freeze

Done.

Copy link
Collaborator

@wojtek-coreum wojtek-coreum left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 22 unresolved discussions (waiting on @miladz68 and @ysv)


x/asset/ft/spec/README.md line 4 at r4 (raw file):

This module called `assetft` is responsible for providing tokenization features for the fungible token. 

Here in this doc we will provide the detailed behavior of fungible tokens as they are implemented. For the fields used in transactions and their meanings please consult the proto files in `coreum/proto/coreum/asset/ft`.

make the path a link maybe?


x/asset/ft/spec/README.md line 17 at r4 (raw file):

Since Coreum is based on Cosmos SDK, We should mention that Cosmos SDK provides the native bank module which is responsible for tracking fungible token creation and balances for each account. But this module does not allow anyone to create a fungible token, mint/burn it, and also does not allow for other features such as freezing and whitelisting. To work around this issue we have wrapped the `bank` module into the `wbank` module. 

In `wbank` module we wrap all the send related  methods of the `bank` module and intercept them with `BeforeSend` and `BeforeInputOutput` functions provided by `assetft` module. This allows `assetft` module to inject custom logic into interceptor functions and reject some transaction if whitelisting or freezing criteria is not met. 

... criteria are not met, or ... criterion is not met


x/asset/ft/spec/README.md line 20 at r4 (raw file):

This structure allows to reuse the code provided by Cosmos SDK, and also reuse the infrastructure that the community provides (e.g explorers and wallets) It is apparent in the query structure. And it also leads to the fact that some of the information regarding fungible tokens will exist in the `assetft` module and some in the `bank` module. For example, if you want to query for frozen balances of a fungible token, you need to query the `assetft` module but if you want to get the total supply, you must query the bank module.

. (dot) after "(e.g explorers and wallets)". (dot) after "(e.g explorers and wallets)"


x/asset/ft/spec/README.md line 20 at r4 (raw file):

This structure allows to reuse the code provided by Cosmos SDK, and also reuse the infrastructure that the community provides (e.g explorers and wallets) It is apparent in the query structure. And it also leads to the fact that some of the information regarding fungible tokens will exist in the `assetft` module and some in the `bank` module. For example, if you want to query for frozen balances of a fungible token, you need to query the `assetft` module but if you want to get the total supply, you must query the bank module.

What does "It is apparent in the query structure." mean?


x/asset/ft/spec/README.md line 28 at r4 (raw file):

# Token Interactions 
## Issue
Coreum provides a decentralized platform which allows everyone to tokenize their assets. Although the functionality of fungible token creation and minting is present in the original `bank` module of Cosmos SDK, it is not exposed to the end users, and it is only possible to create new fungible tokens via either the governance or IBC. The Issue method described here, makes it possible for everyone to create a fungible token and manage its supplies. When the issuer issues a token, they specify the initial total supply which will be delivered to the issuers account address.

issuer's account address


x/asset/ft/spec/README.md line 33 at r4 (raw file):

#### Denom naming, Symbol and Precision
The way that denom is created is that the user provides a name for their subunit, and the denom for the token, which is the main identifier of the token, will be created by appending the subunit with the issuer address separated with a dash (subunit-address). The user also provides the symbol and precision which will only be used for display purposes and will be stored in bank modules metadata field.

"appending the subunit with the issuer address"

I think it should be
"appending the issuer addres to the subunit"

or

"joining the subunit and the issuer address"


x/asset/ft/spec/README.md line 33 at r4 (raw file):

#### Denom naming, Symbol and Precision
The way that denom is created is that the user provides a name for their subunit, and the denom for the token, which is the main identifier of the token, will be created by appending the subunit with the issuer address separated with a dash (subunit-address). The user also provides the symbol and precision which will only be used for display purposes and will be stored in bank modules metadata field.

"bank module's metadata field"


x/asset/ft/spec/README.md line 35 at r4 (raw file):

The way that denom is created is that the user provides a name for their subunit, and the denom for the token, which is the main identifier of the token, will be created by appending the subunit with the issuer address separated with a dash (subunit-address). The user also provides the symbol and precision which will only be used for display purposes and will be stored in bank modules metadata field.

For example to represent bitcoin on Coreum, once could choose satoshi as subunit, BTC as Symbol and 8 as precision. It means that if the issuer address is core1tr3w86yesnj8f290l6ve02cqhae8x4ze0nk0a8 then the denom will be `satoshi-devcore1tr3w86yesnj8f290l6ve02cqhae8x4ze0nk0a8` and since we have chosen BTC as symbol and 8 as precision, it will follow that (1 BTC = 10^8 `satoshi-devcore1tr3w86yesnj8f290l6ve02cqhae8x4ze0nk0a8`)

bitcoin -> Bitcoin


x/asset/ft/spec/README.md line 35 at r4 (raw file):

The way that denom is created is that the user provides a name for their subunit, and the denom for the token, which is the main identifier of the token, will be created by appending the subunit with the issuer address separated with a dash (subunit-address). The user also provides the symbol and precision which will only be used for display purposes and will be stored in bank modules metadata field.

For example to represent bitcoin on Coreum, once could choose satoshi as subunit, BTC as Symbol and 8 as precision. It means that if the issuer address is core1tr3w86yesnj8f290l6ve02cqhae8x4ze0nk0a8 then the denom will be `satoshi-devcore1tr3w86yesnj8f290l6ve02cqhae8x4ze0nk0a8` and since we have chosen BTC as symbol and 8 as precision, it will follow that (1 BTC = 10^8 `satoshi-devcore1tr3w86yesnj8f290l6ve02cqhae8x4ze0nk0a8`)

once -> someone or one


x/asset/ft/spec/README.md line 35 at r4 (raw file):

The way that denom is created is that the user provides a name for their subunit, and the denom for the token, which is the main identifier of the token, will be created by appending the subunit with the issuer address separated with a dash (subunit-address). The user also provides the symbol and precision which will only be used for display purposes and will be stored in bank modules metadata field.

For example to represent bitcoin on Coreum, once could choose satoshi as subunit, BTC as Symbol and 8 as precision. It means that if the issuer address is core1tr3w86yesnj8f290l6ve02cqhae8x4ze0nk0a8 then the denom will be `satoshi-devcore1tr3w86yesnj8f290l6ve02cqhae8x4ze0nk0a8` and since we have chosen BTC as symbol and 8 as precision, it will follow that (1 BTC = 10^8 `satoshi-devcore1tr3w86yesnj8f290l6ve02cqhae8x4ze0nk0a8`)

satoshi-devcore1tr3w86yesnj8f290l6ve02cqhae8x4ze0nk0a8 -> satoshi-core1tr3w86yesnj8f290l6ve02cqhae8x4ze0nk0a8

Code quote:

satoshi-devcore1tr3w86yesnj8f290l6ve02cqhae8x4ze0nk0a8

x/asset/ft/spec/README.md line 38 at r4 (raw file):

#### Token Features
When issuing a token, the issuer must decide which features are enabled on the token. For example if `minting` feature is enabled then it will allow the issuer to mint further tokens on top of the initial supply, otherwise no new tokens can be minted. Here is a list of all the features that can be enabled on the token. Each of these features affects how the token will behave and a detail description will be provided in the dedicated section for each feature.

a detail description -> a detailed description


x/asset/ft/spec/README.md line 46 at r4 (raw file):

#### Burn Rate 
The issuer has the option to provide `BurnRate` when issuing a new token. This values is a number between 0 and 1, and if it is above zero, in every transfer, some additional tokens will be burnt on top of the transfered value, from the senders address. The tokens to be burnt value are calculated by multiplying the transfer amount by burn rate, and rounding it up to an integer value.

This values -> This value


x/asset/ft/spec/README.md line 46 at r4 (raw file):

#### Burn Rate 
The issuer has the option to provide `BurnRate` when issuing a new token. This values is a number between 0 and 1, and if it is above zero, in every transfer, some additional tokens will be burnt on top of the transfered value, from the senders address. The tokens to be burnt value are calculated by multiplying the transfer amount by burn rate, and rounding it up to an integer value.

The tokens to be burnt value are -> The tokens to be burnt are


x/asset/ft/spec/README.md line 46 at r4 (raw file):

#### Burn Rate 
The issuer has the option to provide `BurnRate` when issuing a new token. This values is a number between 0 and 1, and if it is above zero, in every transfer, some additional tokens will be burnt on top of the transfered value, from the senders address. The tokens to be burnt value are calculated by multiplying the transfer amount by burn rate, and rounding it up to an integer value.

by multiplying the transfer amount -> by multiplying the transfered amount


x/asset/ft/spec/README.md line 52 at r4 (raw file):

#### Issuance Fee
Whenever a user wants to issue a fungible token, they have to pay some extra money as issuance fee, which is calculated on type of tx execution fee and will be burnt. The amount of the issuance fee is controlled by governance.

which is calculated on type of -> which is calculated on top of


x/asset/ft/spec/README.md line 55 at r4 (raw file):

## Mint
If the minting feature is enabled, then issuer of the token can submit a Mint transaction to add more tokens to the total supply. All the minted tokens will be transferred to the issuers account address.

issuer's account address


x/asset/ft/spec/README.md line 64 at r4 (raw file):

For example if the issuer freezes 1000 ABC tokens on account Y and this account holds 800, then they cannot move any of their tokens, but if the account receives 400 extra ABC tokens, their total balance will become 1200 and then can only spend 200 of it, since 1000 is Frozen.

Here is the description of behavior of the freezing feature:

also frozen amount can't be burned if burning is enabled


x/asset/ft/spec/README.md line 82 at r4 (raw file):

Here is the description of behavior of the whitelisting feature:
- The issuer can set whitelisted limit on any account.

except their own


x/asset/ft/spec/README.md line 84 at r4 (raw file):

- The issuer can set whitelisted limit on any account.
- The issuer can set whitelisted amount higher or lower than what the user currently holds.
- The issuer account is whitelisted to infinity by default.

and it cannot be modified


x/asset/ft/spec/README.md line 85 at r4 (raw file):

- The issuer can set whitelisted amount higher or lower than what the user currently holds.
- The issuer account is whitelisted to infinity by default.
- The user can receive tokens as long as their total balance after the transaction is not higher that their whitelisted amount

higher than

@ysv ysv requested a review from dzmitryhil January 23, 2023 13:07
Copy link
Contributor

@ysv ysv left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 22 unresolved discussions (waiting on @dzmitryhil and @miladz68)


-- commits line 2 at r1:

Previously, dzmitryhil (Dzmitry Hil) wrote…

@ysv?

yes that is actually the plan to have it in cosmos like style for now but we can skip irrelevant parts (keeper) because we don't provide this module to be used by someone else

IMO the struct for asset/ft should be the following:

  • README - mandator
  • concept - mandatory
  • params - mandatory
  • msgs, events, client - nice to have
  • keeper, state - not needed

e.g feegrant https://docs.cosmos.network/main/modules/feegrant
image.png

Copy link
Contributor

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 21 unresolved discussions (waiting on @miladz68 and @ysv)


-- commits line 2 at r1:

Previously, ysv (Yaroslav Savchuk) wrote…

yes that is actually the plan to have it in cosmos like style for now but we can skip irrelevant parts (keeper) because we don't provide this module to be used by someone else

IMO the struct for asset/ft should be the following:

  • README - mandator
  • concept - mandatory
  • params - mandatory
  • msgs, events, client - nice to have
  • keeper, state - not needed

e.g feegrant https://docs.cosmos.network/main/modules/feegrant
image.png

Ok, resolving this one.

Copy link
Contributor Author

@miladz68 miladz68 left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 21 unresolved discussions (waiting on @wojtek-coreum and @ysv)


x/asset/ft/spec/README.md line 4 at r4 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

make the path a link maybe?

Done.


x/asset/ft/spec/README.md line 17 at r4 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

... criteria are not met, or ... criterion is not met

Done.


x/asset/ft/spec/README.md line 20 at r4 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

. (dot) after "(e.g explorers and wallets)". (dot) after "(e.g explorers and wallets)"

Done.


x/asset/ft/spec/README.md line 20 at r4 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

What does "It is apparent in the query structure." mean?

The sentence should come after the next sentence. But I think it cab be removed.


x/asset/ft/spec/README.md line 28 at r4 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

issuer's account address

Done.


x/asset/ft/spec/README.md line 33 at r4 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

"appending the subunit with the issuer address"

I think it should be
"appending the issuer addres to the subunit"

or

"joining the subunit and the issuer address"

Done.


x/asset/ft/spec/README.md line 33 at r4 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

"bank module's metadata field"

Done.


x/asset/ft/spec/README.md line 35 at r4 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

bitcoin -> Bitcoin

Done.


x/asset/ft/spec/README.md line 35 at r4 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

once -> someone or one

Done.


x/asset/ft/spec/README.md line 35 at r4 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

satoshi-devcore1tr3w86yesnj8f290l6ve02cqhae8x4ze0nk0a8 -> satoshi-core1tr3w86yesnj8f290l6ve02cqhae8x4ze0nk0a8

Done.


x/asset/ft/spec/README.md line 38 at r4 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

a detail description -> a detailed description

Done.


x/asset/ft/spec/README.md line 46 at r4 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

This values -> This value

Done.


x/asset/ft/spec/README.md line 46 at r4 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

The tokens to be burnt value are -> The tokens to be burnt are

Done.


x/asset/ft/spec/README.md line 46 at r4 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

by multiplying the transfer amount -> by multiplying the transfered amount

I want to reference the amout to be transferred, so transfer amount is better than transferred amount. To be very picky, we should write by multiplying the amount to be transferred but it does not read nice.


x/asset/ft/spec/README.md line 52 at r4 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

which is calculated on type of -> which is calculated on top of

Done.


x/asset/ft/spec/README.md line 55 at r4 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

issuer's account address

Done.


x/asset/ft/spec/README.md line 64 at r4 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

also frozen amount can't be burned if burning is enabled

Done.


x/asset/ft/spec/README.md line 82 at r4 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

except their own

Done.


x/asset/ft/spec/README.md line 84 at r4 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

and it cannot be modified

Done.


x/asset/ft/spec/README.md line 85 at r4 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

higher than

added punctuation to make it clear.

Copy link
Collaborator

@wojtek-coreum wojtek-coreum left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @miladz68 and @ysv)


x/asset/ft/spec/README.md line 85 at r4 (raw file):

Previously, miladz68 (milad) wrote…

added punctuation to make it clear.

but still it should be "will not be higher than", no?

Copy link
Contributor Author

@miladz68 miladz68 left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @wojtek-coreum and @ysv)


x/asset/ft/spec/README.md line 85 at r4 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

but still it should be "will not be higher than", no?

I see that I have missed a "be" there.

@ysv ysv requested a review from wojtek-coreum January 23, 2023 17:10
Copy link
Contributor

@ysv ysv left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @miladz68 and @wojtek-coreum)


-- commits line 2 at r1:

Previously, dzmitryhil (Dzmitry Hil) wrote…

Ok, resolving this one.

if you resolve it then I'm blocking because changes are needed IMO

Copy link
Contributor Author

@miladz68 miladz68 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @wojtek-coreum and @ysv)


-- commits line 2 at r1:

Previously, ysv (Yaroslav Savchuk) wrote…

if you resolve it then I'm blocking because changes are needed IMO

@ysv Added Abstract and Concep sections. the rest is not part of the task, and should be done in a separate task.

Copy link
Contributor

@ysv ysv left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @wojtek-coreum)

Copy link
Contributor

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @wojtek-coreum)

Copy link
Collaborator

@wojtek-coreum wojtek-coreum left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @miladz68)


x/asset/ft/spec/README.md line 85 at r4 (raw file):

Previously, miladz68 (milad) wrote…

I see that I have missed a "be" there.

is "that" instead of "than" a correct wording?

Copy link
Contributor Author

@miladz68 miladz68 left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @wojtek-coreum)


x/asset/ft/spec/README.md line 85 at r4 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

is "that" instead of "than" a correct wording?

Done, you are correct.

Copy link
Collaborator

@wojtek-coreum wojtek-coreum left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @dzmitryhil, @miladz68, and @ysv)


x/asset/ft/spec/README.md line 85 at r4 (raw file):

Previously, miladz68 (milad) wrote…

Done, you are correct.

Change wasn't pushed

Copy link
Contributor Author

@miladz68 miladz68 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @dzmitryhil, @wojtek-coreum, and @ysv)


x/asset/ft/spec/README.md line 85 at r4 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

Change wasn't pushed

I don't know why it does not show the changes here, maybe you can check this one in github ?

Copy link
Collaborator

@wojtek-coreum wojtek-coreum left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r9, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @miladz68)


x/asset/ft/spec/README.md line 85 at r4 (raw file):

Previously, miladz68 (milad) wrote…

I don't know why it does not show the changes here, maybe you can check this one in github ?

now I see it

Copy link
Contributor

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r9, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @miladz68)

Copy link
Contributor

@ysv ysv left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r9, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @miladz68)

@miladz68 miladz68 merged commit 83ee5cc into master Jan 26, 2023
@miladz68 miladz68 deleted the milad/asset-ft-docs branch January 26, 2023 10:52
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.

6 participants