Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat: rework proposal messages #2203

Merged
merged 32 commits into from
Aug 28, 2023
Merged

Conversation

robert-zaremba
Copy link
Member

@robert-zaremba robert-zaremba commented Aug 23, 2023

Description

  • remove title and assure to use x/gov metatadata if x/gov authority is used
  • assure to set description otherwise.

Considerations

  • use memo instead of description
    • pro: no need to keep description
    • cons: people usually don't use memo
    • cons: not possible to enforce memo for non x/gov proposals.

Copy link
Collaborator

@gsk967 gsk967 left a comment

Choose a reason for hiding this comment

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

LGTM
Only one suggestion like , if you want remove title and description then remove it , no need for description again for emergency txn , anyway we can retrive from metadata field

@robert-zaremba
Copy link
Member Author

@gsk967 I think description is needed for Emergency Group, because otherwise there is no way to explain the context. I think that should be on chain.

@robert-zaremba robert-zaremba marked this pull request as ready for review August 24, 2023 13:39
@robert-zaremba robert-zaremba requested a review from a team as a code owner August 24, 2023 13:39
@codecov
Copy link

codecov bot commented Aug 24, 2023

Codecov Report

Merging #2203 (6e08423) into main (7f05ad4) will decrease coverage by 5.03%.
Report is 191 commits behind head on main.
The diff coverage is 81.83%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2203      +/-   ##
==========================================
- Coverage   75.38%   70.36%   -5.03%     
==========================================
  Files         100      165      +65     
  Lines        8025    12594    +4569     
==========================================
+ Hits         6050     8862    +2812     
- Misses       1589     3141    +1552     
- Partials      386      591     +205     
Files Changed Coverage Δ
ante/spam_prevention.go 75.92% <ø> (ø)
cmd/ibc_denom/main.go 0.00% <0.00%> (ø)
util/coin/coin.go 12.50% <0.00%> (ø)
x/incentive/codec.go 47.82% <ø> (+9.89%) ⬆️
x/incentive/keeper/invariants.go 0.00% <0.00%> (ø)
x/incentive/keeper/program.go 56.00% <ø> (ø)
x/incentive/keeper/reward.go 82.35% <ø> (ø)
x/incentive/keeper/store.go 70.67% <ø> (ø)
x/incentive/keeper/unbonding.go 80.45% <ø> (ø)
x/incentive/keeper/update.go 52.11% <ø> (ø)
... and 59 more

... and 71 files with indirect coverage changes

@robert-zaremba robert-zaremba requested a review from gsk967 August 24, 2023 13:44
@robert-zaremba robert-zaremba requested a review from a team as a code owner August 24, 2023 13:45
@robert-zaremba robert-zaremba requested a review from toteki August 24, 2023 13:52
Copy link
Collaborator

@gsk967 gsk967 left a comment

Choose a reason for hiding this comment

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

LGTM

@robert-zaremba I need one final confirmation , are you sure all explorers (pingpub/mintscan) showing proposal title and description from metadata feild ?

@robert-zaremba
Copy link
Member Author

@gsk967 pingpub and mintscan yes, other I don't know, they should.
adding Title to the message is not recommended nor documented. In the 0.46 documentation it is said explicitly to use metadata. In 0.47 they readded the fields to the x/gov Proposal.

Copy link
Member

@toteki toteki left a comment

Choose a reason for hiding this comment

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

I have a preference for using memo for emergency groups instead of sometimes using description and sometimes not. It's not a strong preference though

We will have to make sure (during a testnet) that when running this code against existing networks that we don't get problems such as unknown field: title when querying previous MsgGovUpdateRegistry which have that field.

@gsk967
Copy link
Collaborator

gsk967 commented Aug 28, 2023

@toteki title is not deleted, it is reserved, so there will be no issues like unknown field: title on Marshal/Unmarshaling

and description field make sense more than memo field because we already have memo param on txn , so sometimes it will make confuse

@robert-zaremba robert-zaremba added this pull request to the merge queue Aug 28, 2023
Merged via the queue into main with commit 32a8c18 Aug 28, 2023
@robert-zaremba robert-zaremba deleted the robert/ugov-proposal-cleanup branch August 28, 2023 13:31
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants