-
Notifications
You must be signed in to change notification settings - Fork 411
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
Add unpin code flag to store code proposal #979
Add unpin code flag to store code proposal #979
Conversation
Codecov Report
@@ Coverage Diff @@
## main #979 +/- ##
==========================================
- Coverage 59.36% 59.32% -0.04%
==========================================
Files 51 51
Lines 6231 6238 +7
==========================================
+ Hits 3699 3701 +2
- Misses 2266 2271 +5
Partials 266 266
|
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.
Looks good. Only real comment is on the default.
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.
Very nice work! 🏅
Please change the proto field name and cli/ gov handler accordingly to complete this.
x/wasm/keeper/proposal_handler.go
Outdated
@@ -68,7 +68,10 @@ func handleStoreCodeProposal(ctx sdk.Context, k types.ContractOpsKeeper, p types | |||
if err != nil { | |||
return err | |||
} | |||
return k.PinCode(ctx, codeID) | |||
if p.PinCode { |
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.
This would become if !p.Unpinned
(or similar name)
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.
switched to early return instead
// if code should not be pinned return earlier
if p.UnpinCode {
return nil
}
I think it has better readability, double negation logic confuses me a bit personally 😅 but I can change it if there is a preference.
If you can get this fixed up by Wednesday, happy to include this in the v0.29 release. |
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.
Minor issue in the rest client.
Otherwise, this looks great and ready to merge.
Please adjust those fields and mention me
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.
Looking good
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.
🏇 Awesome
This should close #972 if you think it makes sense to have it as part of a governance proposal.