-
Notifications
You must be signed in to change notification settings - Fork 215
MX-15029: Accept multiple delegateVoting #5857
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
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## feat/governance-fixes #5857 +/- ##
======================================================
Coverage 80.32% 80.32%
======================================================
Files 709 709
Lines 93973 94010 +37
======================================================
+ Hits 75480 75515 +35
Misses 13215 13215
- Partials 5278 5280 +2 ☔ View full report in Codecov by Sentry. |
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return onGoingListV2, nil |
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.
You should return onGoingList not the v2
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.
+1
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 method will be called only after GovernanceFixes gets activated, that means there is no ongoing v1 proposal, thus the information is not necessary anymore.
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return onGoingListV2, nil |
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.
+1
| require.False(t, handler.IsFlagEnabled(common.ESDTFlagInSpecificEpochOnly)) // == | ||
| require.True(t, handler.IsFlagEnabled(common.GovernanceFlag)) | ||
| require.False(t, handler.IsFlagEnabled(common.GovernanceFlagInSpecificEpochOnly)) // == | ||
| require.True(t, handler.IsFlagEnabled(common.GovernanceDisableProposeFlag)) // == |
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.
what is the purpose for the == comments?
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.
Will remove in the next PR.
| } | ||
|
|
||
| message DelegatedWithAddress { | ||
| uint64 Nonce = 1; |
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.
any reason we don't add a json tag for nonce, or why we do for the others?
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.
Will add in the next PR.
Reasoning behind the pull request
Proposed changes
Testing procedure