-
Notifications
You must be signed in to change notification settings - Fork 48
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
docs(adr): ADR-21: Subspace specific custom fee tokens #1119
docs(adr): ADR-21: Subspace specific custom fee tokens #1119
Conversation
63342a7
to
a76f613
Compare
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.
I have some doubts on the proposed implementation. I think there's no need for multiple messages and steps. Everything could be simplified to use on-chain proposals instead.
docs/architecture/adr-021-subspace-specific-custom-fee-tokens.md
Outdated
Show resolved
Hide resolved
docs/architecture/adr-021-subspace-specific-custom-fee-tokens.md
Outdated
Show resolved
Hide resolved
docs/architecture/adr-021-subspace-specific-custom-fee-tokens.md
Outdated
Show resolved
Hide resolved
docs/architecture/adr-021-subspace-specific-custom-fee-tokens.md
Outdated
Show resolved
Hide resolved
We will define a new `Params` type to show the `x/subspaces` parameters. | ||
|
||
```proto | ||
// Params contains the module parameters | ||
message Params { | ||
// List of the minimum gas prices to be accepted by validators | ||
repeated Coin min_gas_prices = 2; | ||
} | ||
``` |
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.
Why is this needed? Each subspace will have its own allowed_fee_tokens
list anyway. There's no need to have a module-wide list
docs/architecture/adr-021-subspace-specific-custom-fee-tokens.md
Outdated
Show resolved
Hide resolved
docs/architecture/adr-021-subspace-specific-custom-fee-tokens.md
Outdated
Show resolved
Hide resolved
docs/architecture/adr-021-subspace-specific-custom-fee-tokens.md
Outdated
Show resolved
Hide resolved
docs/architecture/adr-021-subspace-specific-custom-fee-tokens.md
Outdated
Show resolved
Hide resolved
Co-authored-by: Riccardo <riccardo.montagnin@gmail.com>
Co-authored-by: Riccardo <riccardo.montagnin@gmail.com>
Co-authored-by: Riccardo <riccardo.montagnin@gmail.com>
Co-authored-by: Riccardo <riccardo.montagnin@gmail.com>
Co-authored-by: Riccardo <riccardo.montagnin@gmail.com>
Co-authored-by: Riccardo <riccardo.montagnin@gmail.com>
Params params = 1; | ||
} | ||
``` | ||
### Update `x/fees` |
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.
Since x/fees may conflict with subspace fee token checks, I think we can update the logic of CheckFees
only check dsm
. What do you think? @RiccardoM @manu0466
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.
I think we should completely drop the x/fees
module instead. It's never been used anyway
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.
@RiccardoM Updated docs.
docs/architecture/adr-021-subspace-specific-custom-fee-tokens.md
Outdated
Show resolved
Hide resolved
docs/architecture/adr-021-subspace-specific-custom-fee-tokens.md
Outdated
Show resolved
Hide resolved
docs/architecture/adr-021-subspace-specific-custom-fee-tokens.md
Outdated
Show resolved
Hide resolved
docs/architecture/adr-021-subspace-specific-custom-fee-tokens.md
Outdated
Show resolved
Hide resolved
The solution outlined above is **not** backwards compatible and will require a migration script to update all existing subspaces to the new version. This script will handle the following tasks: | ||
- migrate all subspaces to have a default allowed fee tokens list. |
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.
The solution outlined above is **not** backwards compatible and will require a migration script to update all existing subspaces to the new version. This script will handle the following tasks: | |
- migrate all subspaces to have a default allowed fee tokens list. | |
The solution outlined above is **not** backward compatible, and it requires a migration script to update all existing subspaces to the new version. The script should take care of migrating all subspaces to have a default allowed fee tokens list. |
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.
Is this actually true though? If we mark the Proto field as optional
, do we need to migrate everything?
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.
Now proto3 fields are all optional now. You are right, adding new field does not cause the backward compatibility issues. I just learned from the official documentation. Thanks!
https://protobuf.dev/programming-guides/proto3/#updating
If you add new fields, any messages serialized by code using your “old” message format can still be parsed by your new generated code. You should keep in mind the default values for these elements so that new code can properly interact with messages generated by old code. Similarly, messages created by your new code can be parsed by your old code: old binaries simply ignore the new field when parsing. See the Unknown Fields section for details.
Co-authored-by: Riccardo <riccardo.montagnin@gmail.com>
Co-authored-by: Riccardo <riccardo.montagnin@gmail.com>
Co-authored-by: Riccardo <riccardo.montagnin@gmail.com>
78842e9
to
c433739
Compare
c433739
to
8073448
Compare
Description
Closes: #XXXX
This PR introduces the subspace specific custom fee tokens by the ADR.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!
to the type prefix if API or client breaking changeCHANGELOG.md
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!
in the type prefix if API or client breaking change