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

feat!: implement ADR 21: Subspace specific custom fee tokens #1138

Merged
merged 31 commits into from
Jun 15, 2023

Conversation

dadamu
Copy link
Contributor

@dadamu dadamu commented May 10, 2023

Description

This PR implements ADR-21: Subspace specific custom fee tokens feature.

Closes: DCD-312

Depends-On: #1135
Depends-On: #1139


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...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

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...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@github-actions github-actions bot added kind/ci Improve the CI/CD x/CLI x/fees x/profiles Module that allows to create and manage decentralized social profiles x/relationships x/subspaces Issue on the x/subspaces module labels May 10, 2023
@dadamu dadamu force-pushed the paul/DCD-312/implement-adr-21 branch from 3d9715a to 92e4ff9 Compare May 11, 2023 10:16
@codecov
Copy link

codecov bot commented May 11, 2023

Codecov Report

Patch coverage: 83.22% and project coverage change: -0.46 ⚠️

Comparison is base (f0b03d0) 80.77% compared to head (cecf756) 80.32%.

❗ Current head cecf756 differs from pull request most recent head 5f0d45f. Consider uploading reports for the commit 5f0d45f to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1138      +/-   ##
==========================================
- Coverage   80.77%   80.32%   -0.46%     
==========================================
  Files         193      190       -3     
  Lines       17159    16825     -334     
==========================================
- Hits        13861    13515     -346     
- Misses       2701     2716      +15     
+ Partials      597      594       -3     
Impacted Files Coverage Δ
x/reactions/types/msgs.go 95.16% <0.00%> (-2.10%) ⬇️
x/reports/types/msgs.go 96.98% <0.00%> (-1.79%) ⬇️
x/subspaces/ante/fee.go 82.35% <0.00%> (ø)
x/subspaces/types/msgs_feegrant.go 82.47% <0.00%> (-1.74%) ⬇️
x/subspaces/types/msgs_treasury.go 93.75% <0.00%> (-3.03%) ⬇️
x/subspaces/types/msgs.go 94.70% <65.00%> (-2.88%) ⬇️
x/subspaces/types/codec.go 58.66% <85.71%> (+2.78%) ⬆️
x/subspaces/ante/tx_checker.go 100.00% <100.00%> (ø)
x/subspaces/keeper/keeper.go 100.00% <100.00%> (ø)
x/subspaces/keeper/msg_server.go 88.98% <100.00%> (+0.57%) ⬆️
... and 2 more

... and 14 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@dadamu dadamu changed the title feat!: implement ADR 21 feat!: implement ADR 21: Subspace specific custom fee tokens May 12, 2023
@dadamu dadamu marked this pull request as ready for review May 12, 2023 08:06
@dadamu dadamu requested a review from a team as a code owner May 12, 2023 08:06
@dadamu dadamu force-pushed the paul/DCD-312/implement-adr-21 branch 4 times, most recently from 8aa800d to 828775f Compare May 17, 2023 06:04
@dadamu
Copy link
Contributor Author

dadamu commented May 17, 2023

@RiccardoM @manu0466 Rebased

@dadamu dadamu force-pushed the paul/DCD-312/implement-adr-21 branch 2 times, most recently from 28719f5 to 5f5e86e Compare May 19, 2023 15:35
@dadamu dadamu force-pushed the paul/DCD-312/implement-adr-21 branch from 5f5e86e to 533416b Compare June 5, 2023 07:27
proto/desmos/subspaces/v3/msgs.proto Outdated Show resolved Hide resolved
proto/desmos/subspaces/v3/msgs.proto Outdated Show resolved Hide resolved
x/subspaces/ante/tx_checker.go Show resolved Hide resolved
Comment on lines 22 to 44
newMinPrices := MergeMinPrices(ctx.MinGasPrices(), sdk.NewDecCoinsFromCoins(subspace.AllowedFeeTokens...))
newCtx := ctx.WithMinGasPrices(newMinPrices)
return txFeeChecker(newCtx, tx)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not super familiar with on-chain gas prices, but I thought they were treated using the and logic. So if I have a min gas price of 0.01udsm,0.01ustar then to properly broadcast my transaction I have to pay fees in both tokens, and not either one of the two. Do you have any link for this to be checked out that we can paste here to remember this in the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are treated as or, so one of tokens reaches the min fees can broadcast properly, say, to send the message with 100 gas via the node with 0.01udsm, 0.01ustar min gas prices, 1udsm or 1ustar can pass the check.

Here is the reference:
https://github.com/cosmos/cosmos-sdk/blob/release/v0.47.x/x/auth/ante/validator_tx_fee.go#L38

x/subspaces/client/cli/cli_test.go Outdated Show resolved Hide resolved
x/subspaces/keeper/msg_server.go Outdated Show resolved Hide resolved
x/subspaces/keeper/msg_server_test.go Outdated Show resolved Hide resolved
// Set allowed fee tokens without validating it.
// Before storing the updated subspace, a validation with Validate() should
// be performed.
func (sub Subspace) SetAllowedFeeTokens(feeTokens sdk.Coins) Subspace {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not a big fan of this approach. Instead I think it would be better to:

  1. Add another param inside the NewSubspace function to specify the fee tokens when creating the subspace. This is going to be useful when exporting/importing the genesis as well as inside tests.
  2. Rename this method Update as we have done with the Post and Attachment type, to keep everything consistent.

Also, I'm doubting whether AllowedFeeTokens is the proper name. I think AdditionalFeeTokens might be better instead. The name AllowedFeeTokens might make people think that if I specify 0.01ustar then the star token is going to be then only one allowed, while the udsm should always be allowed anyway. Using AdditionalFeeTokens might make everything more easy to understand.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, I use the SetAllowedFeeTokens since adding another param inside the NewSubspace will break hundreds of existing test codes, but yeah, it is better to have an additional field.

strongly agree with AdditionalFeeTokens is better.

Copy link
Contributor Author

@dadamu dadamu Jun 13, 2023

Choose a reason for hiding this comment

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

Finally, I used sed to add the new parameter quickly:

#!/bin/bash

dir="./x"

find "$dir" -type f -name "*.go" | while read -r file; do
    sed -i -E ':a;N;$!ba;s/NewSubspace\([^)]*time.UTC,/&\nnil,/g' "$file"
    echo "$file"
done

Is any other tools to make modifying codes quickly?

Copy link
Contributor

Choose a reason for hiding this comment

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

I honestly don't know of any other tool that can easily update the code. I would have done it by hand 😓 Great job using sed 💯

@dadamu dadamu force-pushed the paul/DCD-312/implement-adr-21 branch from cecf756 to e674ee7 Compare June 15, 2023 07:32
Copy link
Contributor

@RiccardoM RiccardoM left a comment

Choose a reason for hiding this comment

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

Great job 💯

@RiccardoM RiccardoM merged commit e045cda into master Jun 15, 2023
1 check passed
@RiccardoM RiccardoM deleted the paul/DCD-312/implement-adr-21 branch June 15, 2023 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
x/CLI x/profiles Module that allows to create and manage decentralized social profiles x/subspaces Issue on the x/subspaces module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants