-
Notifications
You must be signed in to change notification settings - Fork 18
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
feat: implement approve oracle upgrade tx #582
Conversation
Co-authored-by: Youngjoon Lee <[email protected]>
…nto ft/569/oracle-upgrade-handler # Conflicts: # cmd/panacead/cmd/genoracle.go # proto/panacea/oracle/v2/oracle.proto
…nto ft/570/upgrade-endblocker
…ft/571/upgrade-oracle-tx # Conflicts: # proto/panacea/oracle/v2/oracle.proto # proto/panacea/oracle/v2/proposal.proto # proto/panacea/oracle/v2/query.proto # x/oracle/types/errors.go # x/oracle/types/keys.go # x/oracle/types/oracle.go # x/oracle/types/oracle.pb.go # x/oracle/types/query.pb.go # x/oracle/types/query.pb.gw.go
…ft/575/upgrade-proto # Conflicts: # proto/panacea/oracle/v2/oracle.proto # proto/panacea/oracle/v2/proposal.proto # proto/panacea/oracle/v2/query.proto # x/oracle/types/keys.go # x/oracle/types/oracle.pb.go # x/oracle/types/query.pb.go # x/oracle/types/query.pb.gw.go
This reverts commit 44a3f47.
…upgrade" This reverts commit f73a2ca.
…1/upgrade-oracle-tx # Conflicts: # proto/panacea/oracle/v2/oracle.proto # proto/panacea/oracle/v2/proposal.proto # x/oracle/keeper/grpc_query_oracle.go # x/oracle/types/keys.go # x/oracle/types/oracle.pb.go
if err := oracleRegistration.ValidateBasic(); err != nil { | ||
return err | ||
} | ||
|
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.
As commented in the previous PR, I deleted this validation.
@@ -133,10 +132,11 @@ func (suite *oracleTestSuite) TestRegisterOracleSuccess() { | |||
suite.Require().Equal(suite.oracleCommissionMaxChangeRate, oracleFromKeeper.OracleCommissionMaxChangeRate) | |||
} | |||
|
|||
func (suite *oracleTestSuite) TestRegisterOracleFailedValidateToMsgOracleRegistration() { |
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.
Accordingly this, deleted validation test code.
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.
Thanks for your hard work. I have one comment.
x/oracle/keeper/oracle.go
Outdated
if err != nil || !existing { | ||
return fmt.Errorf("failed to check if the approver oracle exists or not. address(%s)", approval.ApproverOracleAddress) |
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.
How about splitting the error? It seems that the address may not be valid and the oracle may not be in the store.
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.
Got it. 1c69a6d
As you can see the commit, I changed the HasOracle
function.
Because we should check if the oracle exists and is active as well. Thank you for pointing out.
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.
LGTM
), | ||
) | ||
|
||
// TODO: add to queue(?) for update unique ID |
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.
As discussed offline, it seems that you can check if the encryptedOraclePrivKey of OracleUpgrade is not nil without putting it in the queue.
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.
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.
Okay, I understand. I will reflect it in PR #584.
Co-authored-by: Hansol Lee <[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.
lgtm
…oracle-reg # Conflicts: # proto/panacea/oracle/v2/tx.proto # x/oracle/abci.go # x/oracle/abci_test.go # x/oracle/keeper/upgrade.go # x/oracle/types/message_oracle.go # x/oracle/types/oracle.go # x/oracle/types/tx.pb.go
… into ft/574/approve-oracle-upgrade # Conflicts: # x/oracle/keeper/upgrade.go # x/oracle/types/errors.go
…-upgrade # Conflicts: # proto/panacea/oracle/v2/tx.proto # x/oracle/keeper/oracle.go # x/oracle/types/tx.pb.go
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.
lgtm
close #574
Implemented
MsgApproveOracleUpgrade
transaction with some refactoring.