-
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: add oracle upgrade endblocker #578
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
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
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
x/oracle/abci.go
Outdated
if err := keeper.ApplyUpgrade(ctx, upgradeInfo); err != nil { | ||
panic(err) | ||
} | ||
keeper.RemoveOracleUpgradeInfo(ctx) |
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 not deleting the upgrade info?
When a new upgrade proposal passes, it will be overwritten anyway.
The reason I'm talking about this is because of the following case:
If the oracles didn't upgrade until the target height, they had to re-request register-oracle
transaction again to be shared the oracle private key in the new binary(upgrade version).
However, at this time, the oracles will be able to enter the commission rate, which overwrites the existing oracle commission rate.
In other words, this case should also be done through upgrade-oracle
, and to do so, it seems that upgrade info should be left. upgrade info will mean upcoming or recent upgrade information.
--
FYI) Plus, in register-oracle
, the check that if the oracle has already been set or not should be added.
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.
That's a good idea.
Then, how about OracleKeyMigration
rather than a name OracleUpgrade
?
OracleKeyMigration works only when it has the same value as uniqueID of OracleUpgradeInfo
.
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.
Oh, please forget the name change. I'll think about it 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 the problem.
If the oracles didn't upgrade until the target height, oracle operator have to use upgrade-oracle
because the oracle is already in Oracle
store.
For now, I think @H4NLee 's suggestion is the best way.
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.
If you reflect this flow, please leave a comment :)
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 fixed it in 8e1a3d8.
…cker # Conflicts: # proto/panacea/oracle/v2/query.proto # x/oracle/keeper/upgrade.go # x/oracle/keeper/upgrade_test.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
Please check my review.
Co-authored-by: gyuguen <[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
closes #570 .
Oracle
inApplyOracle