Skip to content

[eth] contract improvement #348

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

Merged
merged 4 commits into from
Oct 17, 2022
Merged

Conversation

ali-behjati
Copy link
Collaborator

@ali-behjati ali-behjati commented Oct 14, 2022

This PR improves the eth contract by some small changes.

  • Renames Pyth.initializeto Pyth._initialize and makes it internal. This method was public and always was confusing although technically it was safe (PythUpgradable overridden it).
  • Makes contract upgrade more resillient + add fails test: There is a pythUpgradableMagic method that returns some random number and the upgrades makes sure that this function can be called and it can return the correct value. There is a test to capture it.
    • Unfortunately I couldn't add an increasing version check as string comparison does not exist in solidity (for equality you should hash the string!)
  • Removes deploy commit hash. It is very error-prone as not used anywhere. I could add it to the upgrades, but that requires a lot of change and additional overhead to again inject it in the migration step (which we should make sure we are in the right branch). We can add it later if needed.
  • Improves the price not found log. This error could happen for an existing price that hasn't received any update on target chain yet. Modified the log to be more verbose.

The deployCommitHash process is error-prone and it's alternatives
require changing many parts of the code.
And as it is not used anywhere. I believe it is not
worth the effort.
@ali-behjati ali-behjati changed the title Abehjati/eth-contract-improvement [eth] contract improvement Oct 14, 2022
Copy link
Contributor

@jayantk jayantk left a comment

Choose a reason for hiding this comment

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

lgtm modulo the comment. I think it would be good to have another review from someone with better solidity knowledge than me as well.

Copy link
Contributor

@njk-64 njk-64 left a comment

Choose a reason for hiding this comment

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

lgtm

@ali-behjati ali-behjati requested a review from jayantk October 17, 2022 11:38
@ali-behjati ali-behjati merged commit c47199d into main Oct 17, 2022
@ali-behjati ali-behjati deleted the abehjati/eth-contract-improvement branch October 17, 2022 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants