desmos-labs / desmos

Improving the well-being of users on social networks through the blockchain technology.
https://desmos.network
Apache License 2.0
103 stars 47 forks source link

fix: exclude subspace management messages from subspace feegrant targets #1152

Closed dadamu closed 1 year ago

dadamu commented 1 year ago

Description

Closes: #XXXX This PR exclude subspace management messages from subspace feegrant since subspace feegrant should only target to the social related messages.

The changes includes:

  1. Rename SubspaceMsg into SocialMsg
  2. Add IsSocialMsg function to represent if the message is a social message.

The social messages are:


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

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

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 36.36% and project coverage change: -0.07 :warning:

Comparison is base (6827291) 80.84% compared to head (2ee63c0) 80.78%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1152 +/- ## ========================================== - Coverage 80.84% 80.78% -0.07% ========================================== Files 187 187 Lines 16802 16816 +14 ========================================== Hits 13584 13584 - Misses 2622 2636 +14 Partials 596 596 ``` | [Impacted Files](https://app.codecov.io/gh/desmos-labs/desmos/pull/1152?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=desmos-labs) | Coverage Δ | | |---|---|---| | [x/posts/types/msgs.go](https://app.codecov.io/gh/desmos-labs/desmos/pull/1152?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=desmos-labs#diff-eC9wb3N0cy90eXBlcy9tc2dzLmdv) | `94.94% <0.00%> (-2.27%)` | :arrow_down: | | [x/reactions/types/msgs.go](https://app.codecov.io/gh/desmos-labs/desmos/pull/1152?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=desmos-labs#diff-eC9yZWFjdGlvbnMvdHlwZXMvbXNncy5nbw==) | `96.19% <0.00%> (-1.06%)` | :arrow_down: | | [x/relationships/types/msgs.go](https://app.codecov.io/gh/desmos-labs/desmos/pull/1152?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=desmos-labs#diff-eC9yZWxhdGlvbnNoaXBzL3R5cGVzL21zZ3MuZ28=) | `91.59% <0.00%> (-3.19%)` | :arrow_down: | | [x/reports/types/msgs.go](https://app.codecov.io/gh/desmos-labs/desmos/pull/1152?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=desmos-labs#diff-eC9yZXBvcnRzL3R5cGVzL21zZ3MuZ28=) | `97.57% <0.00%> (-1.20%)` | :arrow_down: | | [x/subspaces/types/msgs.go](https://app.codecov.io/gh/desmos-labs/desmos/pull/1152?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=desmos-labs#diff-eC9zdWJzcGFjZXMvdHlwZXMvbXNncy5nbw==) | `97.57% <ø> (ø)` | | | [x/subspaces/ante/fee.go](https://app.codecov.io/gh/desmos-labs/desmos/pull/1152?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=desmos-labs#diff-eC9zdWJzcGFjZXMvYW50ZS9mZWUuZ28=) | `82.35% <100.00%> (ø)` | | | [x/subspaces/authz/authz.go](https://app.codecov.io/gh/desmos-labs/desmos/pull/1152?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=desmos-labs#diff-eC9zdWJzcGFjZXMvYXV0aHovYXV0aHouZ28=) | `92.59% <100.00%> (ø)` | |

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

RiccardoM commented 1 year ago

@dadamu I honestly don't understand the rational behind this reason. Why shouldn't we allow subspace administrators to grant other administratos the option to pay for transaction fees when they perform these kind of messages? I think reducing the number of messages supported by SubspaceFeeGrant only reduces the number of use cases we support, without any actualy benefit. Could you expand on why we want to do that and what pros we might get from this?

dadamu commented 1 year ago

@RiccardoM This idea comes from that subspace AllowedFeeTokens target messages should exclude the subspace management messages. Honestly, I just think the AllowedFeeTokens situation might be the same as subspace feegrant and authz, making them focus on the social messages, excluding management messages. With this PR, we can reuse SocialMsg inside the AllowedFeeTokens feature.

RiccardoM commented 1 year ago

@RiccardoM This idea comes from that subspace AllowedFeeTokens target messages should exclude the subspace management messages. Honestly, I just think the AllowedFeeTokens situation might be the same as subspace feegrant and authz, with this PR, we can reuse SocialMsg inside the AllowedFeeTokens feature.

Still, I don't understand what advantages this change will have:

Unless there's a very good reason why this should be implemented, I'm against this change as I see more cons than pros. Even if there were good reasons, since this is is an important (breaking) change, it should be first discussed into an ADR to keep track of this.

dadamu commented 1 year ago

@RiccardoM I see, after rethinking about the PR, target messages to subspace AllowedFeeTokens should be different from subspaces feegrant and authz. There is no advantage with this change. As a result, the PR should be close.