-
Notifications
You must be signed in to change notification settings - Fork 109
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
refactor: authorizations table #2114
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2114 +/- ##
===========================================
+ Coverage 72.34% 72.40% +0.05%
===========================================
Files 250 257 +7
Lines 14046 14214 +168
===========================================
+ Hits 10162 10291 +129
- Misses 3423 3459 +36
- Partials 461 464 +3
|
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.
Not complete review but giving some comments already
Co-authored-by: Lucas Bertrand <[email protected]>
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, just some comments to consider before merging
return false | ||
} | ||
|
||
var OperationalPolicyMessageList = []string{ |
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.
I'm thinking about defining a type PolicyTable
containing a list [url,policy] tuples with crud methods, so it can be easily update, even if we add a new policy type and can be used once we persist the table in the state
|
||
var OperationalPolicyMessageList = []string{ | ||
// Crosschain admin messages | ||
sdk.MsgTypeURL(&crosschaintypes.MsgRefundAbortedCCTX{}), |
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.
Can be made less verbose defining an alias
msgURL := sdk.MsgTypeURL
"github.com/zeta-chain/zetacore/pkg/authorizations" | ||
authoritytypes "github.com/zeta-chain/zetacore/x/authority/types" | ||
) | ||
|
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.
We can add assertion with all messages in the tests
@@ -0,0 +1,59 @@ | |||
package types |
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.
The test for this file use the pause message
Description
MsgUpdateZRC20
intoMsgPauseZRC20
andMsgUnPauseZRC20
MsgUpdateCrosschainFlags
intoMsgEnableCCTXFlags
MsgDisableCCTXFlags
andMsgUpdateGasPriceIncreaseFlags
MsgPauseZRC20 : PolicyType_groupEmergency
MsgUnPauseZRC20 PolicyType_groupOperational
MsgUpdateGasPriceIncreaseFlags : PolicyType_groupOperational
MsgEnableCCTXFlags : PolicyType_groupOperational
MsgDisableCCTXFlags : PolicyType_groupEmergency
Closes:#2107
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Include instructions and any relevant details so others can reproduce.
Checklist: