-
Notifications
You must be signed in to change notification settings - Fork 27
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
Conversation
There was a problem hiding this 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:complete! all files reviewed, all discussions resolved (waiting on @dzmitryhil, @wojtek-coreum, and @ysv)
There was a problem hiding this 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.
There was a problem hiding this 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)
There was a problem hiding this 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 the
bank 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.
There was a problem hiding this 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)
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 thenft
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 beresponsible 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 justregarding
This sentence doesn't sound goodFor 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 the
bankmodule.
How about: For example, if you want to query for frozen balances of a fungible token, you need to query theassetft
module but if you want to get the total supply, you must query thebank
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 theissuance 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 theissuance 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
· andSend Commission Rate
should be after the Features, but before theIssuance 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 theapply
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.
There was a problem hiding this 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
There was a problem hiding this 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)
There was a problem hiding this 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.
There was a problem hiding this 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.
There was a problem hiding this 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
There was a problem hiding this 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
There was a problem hiding this 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @dzmitryhil, @miladz68, @wojtek-coreum, and @ysv)
There was a problem hiding this 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
There was a problem hiding this 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)
Previously, miladz68 (milad) wrote…
I argued that having it in that format brings no real value.
@ysv?
There was a problem hiding this 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.
There was a problem hiding this 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
There was a problem hiding this 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)
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
There was a problem hiding this 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)
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
Ok, resolving this one.
There was a problem hiding this 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.
There was a problem hiding this 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?
There was a problem hiding this 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.
There was a problem hiding this 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)
Previously, dzmitryhil (Dzmitry Hil) wrote…
Ok, resolving this one.
if you resolve it then I'm blocking because changes are needed IMO
There was a problem hiding this 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)
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.
There was a problem hiding this 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)
There was a problem hiding this 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)
There was a problem hiding this 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?
There was a problem hiding this 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.
There was a problem hiding this 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
There was a problem hiding this 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 ?
There was a problem hiding this 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: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
There was a problem hiding this 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:complete! all files reviewed, all discussions resolved (waiting on @miladz68)
There was a problem hiding this 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:complete! all files reviewed, all discussions resolved (waiting on @miladz68)
This change is![Reviewable](https://camo.githubusercontent.com/1541c4039185914e83657d3683ec25920c672c6c5c7ab4240ee7bff601adec0b/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)