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(e2e): create FeeOracleV2 deployer #2477

Merged
merged 2 commits into from
Nov 14, 2024
Merged

feat(e2e): create FeeOracleV2 deployer #2477

merged 2 commits into from
Nov 14, 2024

Conversation

Zodomo
Copy link
Contributor

@Zodomo Zodomo commented Nov 14, 2024

Created a FeeOracleV2 deployment routine that uses Create3. We cannot upgrade FeeOracleV1 as V2 has a different storage layout. This routine does not currently include upgrade logic.

issue: #1951

@Zodomo Zodomo self-assigned this Nov 14, 2024
kevinhalliday
kevinhalliday reviewed Nov 14, 2024
View reviewed changes
Copy link
Contributor

@kevinhalliday kevinhalliday left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Overall looks pretty good.

There are a couple of FeeOracleV1 references left in comments.

lib/contracts/feeoraclev2/feeparams.go Outdated Show resolved Hide resolved
lib/contracts/feeoraclev2/deploy.go Outdated Show resolved Hide resolved
lib/contracts/feeoraclev2/feeparams.go Outdated Show resolved Hide resolved
lib/contracts/feeoraclev2/feeparams.go Show resolved Hide resolved
chmllr
chmllr approved these changes Nov 14, 2024
View reviewed changes
Copy link
Contributor

@chmllr chmllr left a comment

Choose a reason for hiding this comment

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

Code LGTM, can't really review the semantics yet.

kc1116
kc1116 reviewed Nov 14, 2024
View reviewed changes
e2e/app/feeoraclev2.go Outdated Show resolved Hide resolved
kc1116
kc1116 reviewed Nov 14, 2024
View reviewed changes
e2e/app/feeoraclev2.go Outdated Show resolved Hide resolved
kc1116
kc1116 reviewed Nov 14, 2024
View reviewed changes
e2e/app/feeoraclev2.go Outdated Show resolved Hide resolved
kc1116
kc1116 reviewed Nov 14, 2024
View reviewed changes
Copy link
Contributor

@kc1116 kc1116 left a comment

Choose a reason for hiding this comment

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

Left some small Go comments

@Zodomo
applied feedback
1174404 
data gas price retrieved from postsTo chain, increased base gas limit, set FeeParams for src chain on src chain, condensed unexported Deploy function into the public one, added godocs, migrated parsing loop to helper function, and applied efficiency suggestions
@Zodomo Zodomo merged commit daed86a into main Nov 14, 2024
19 checks passed
@Zodomo Zodomo deleted the zodomo/deploy-feeoraclev2 branch November 14, 2024 23:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kevinhalliday kevinhalliday kevinhalliday approved these changes

@corverroos corverroos Awaiting requested review from corverroos corverroos is a code owner

@sideninja sideninja Awaiting requested review from sideninja

@chmllr chmllr Awaiting requested review from chmllr chmllr is a code owner

@kc1116 kc1116 Awaiting requested review from kc1116 kc1116 is a code owner

Assignees

@Zodomo Zodomo

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Zodomo @chmllr @kc1116 @kevinhalliday