Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: add subscription and handler for upgrade oracle event #61

Merged
merged 6 commits into from
Jan 3, 2023
Merged

feat: add subscription and handler for upgrade oracle event #61

merged 6 commits into from
Jan 3, 2023

Conversation

0xHansLee
Copy link
Contributor

close #58

With implementation, I did some refactoring:

  • In verification of remote report, remove checking product ID. It seems that only checking unique ID is enough.
  • Move verifyTrustedBlockInfo to the function of verifiedQueryClient struct.
  • Refactor and split some functions that were in event/oracle/event.go to reuse in OracleUpgradeEvent.

@0xHansLee 0xHansLee self-assigned this Dec 28, 2022
@0xHansLee 0xHansLee linked an issue Dec 29, 2022 that may be closed by this pull request
Subscribe UpgradeOracle event #58
Closed
audtlr24
audtlr24 approved these changes Dec 30, 2022
View reviewed changes
Copy link
Contributor

@audtlr24 audtlr24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

gyuguen
gyuguen approved these changes Jan 3, 2023
View reviewed changes
Copy link
Contributor

@gyuguen gyuguen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you.
I have an one comment.

event/oracle/upgrade_oracle_event.go
approvalMsg := &oracletypes.ApprovalSharingOracleKey{
UniqueId: approverUniqueID,
ApproverOracleAddress: approverAddress,
TargetOracleAddress: targetAddress,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to targetOracleUniqueID field in ApprovalSharingOracleKey struct.
This message should include the uniqueID of the target to be upgraded so that panacea-core can find it.
That is, it is a confirmation that this request is for the current upgrade target.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's good point. It'd be better to include target unique ID and compare it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll make an issue in both core and oracle, and then handle it as another PR.
Thank you for your comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, if the targetOracleUniqueID is to enable panacea to find unique ID, there is no need to add it.
panacea can get(find) unique ID of target uniqueID using GetOracleUpgradeInfo.
I'll think a little more about whether this is really necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my opinion, as @gyuguen said, it would be better to include TargetOracleUniqueID in ApprovalSharingOracleKey struct.

It's not a critical point because oracle checks the unique ID from upgrade-oracle event is equal to the current unique ID of upgrade version of oracle.
However, assuming that oracle and core do not believe each other, the unique ID check should also been done in core side.

Copy link
Contributor

@gyuguen gyuguen Jan 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. As you said, the value can be got from OracleUpgradeInfo.
But I think we will check if the request for Approval is really the uniqueID currently upgrading to.
If it is determined that this validation is not important, the TargetOracleUniqueID need not be added. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also think it's good to add.
made an issue 👍

Base automatically changed from ft/57/subscribi-approve-upgrade to main January 3, 2023 05:56
inchori
inchori approved these changes Jan 3, 2023
View reviewed changes
Copy link
Contributor

@inchori inchori left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

H4NLee added 2 commits January 3, 2023 15:58
Merge remote-tracking branch 'origin/main' into ft/58/subscribe-upgra…
ce415a9 
…de-oracle

# Conflicts:
#	go.mod
#	go.sum
#	panacea/query_client.go
#	server/middleware/auth_test.go
merge
108c845
@0xHansLee 0xHansLee force-pushed the ft/58/subscribe-upgrade-oracle branch from 62e1b4d to 108c845 Compare January 3, 2023 07:12
@0xHansLee 0xHansLee mentioned this pull request Jan 3, 2023
Closed
@0xHansLee 0xHansLee merged commit fe97e9e into main Jan 3, 2023
@0xHansLee 0xHansLee deleted the ft/58/subscribe-upgrade-oracle branch January 3, 2023 07:51
@0xHansLee 0xHansLee mentioned this pull request Jan 3, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gyuguen gyuguen gyuguen approved these changes

@audtlr24 audtlr24 audtlr24 approved these changes

@inchori inchori inchori approved these changes

@youngjoon-lee youngjoon-lee Awaiting requested review from youngjoon-lee

Assignees

@0xHansLee 0xHansLee

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Subscribe UpgradeOracle event
4 participants
@0xHansLee @gyuguen @audtlr24 @inchori