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

fix: lower priority fee #2148

Merged
merged 15 commits into from
Oct 21, 2024
Merged

fix: lower priority fee #2148

merged 15 commits into from
Oct 21, 2024

Conversation

alxiong
Copy link
Contributor

@alxiong alxiong commented Oct 10, 2024 •
edited
Loading

Closes #2147

This PR:

  • Switch to the right version of PlonkVerifier (in the deployer.rs we incorrectly deployed PlonkVerifier2.sol which is secure but less efficient)
  • Use lower priority fee based on live gas oracle when sending out light client update transaction

Comments

  • there is no dedicated price oracle for testnet (e.g. sepolia.etherscan.io), to avoid complicated logic, we simply keep using the same mainnet stats-informed gas pricing.

sveitser
sveitser reviewed Oct 10, 2024
View reviewed changes
utils/src/deployer.rs Outdated Show resolved Hide resolved
@alxiong alxiong changed the base branch from release-gambit to main October 10, 2024 17:46
@alxiong alxiong force-pushed the gambit-contract-patch branch from 581d950 to 1ad78b9 Compare October 10, 2024 20:15
sveitser
sveitser requested changes Oct 10, 2024
View reviewed changes
hotshot-state-prover/src/service.rs Outdated Show resolved Hide resolved
sveitser
sveitser reviewed Oct 11, 2024
View reviewed changes
hotshot-state-prover/src/service.rs Outdated
.unwrap()
.into();
inner.max_fee_per_gas = Some(safe_max_fee);
inner.max_priority_fee_per_gas = Some(safe_max_fee - base_fee);
Copy link
Collaborator

Choose a reason for hiding this comment

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

@alxiong It looks like this could underflow it would probably lead to the transaction being rejected because we don't have the funds but let's not rely on that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see if you like 6106914

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is outdated, see the latest commits that use blocknative and trait PriceOracle from ethers-rs

sveitser
sveitser reviewed Oct 11, 2024
View reviewed changes
hotshot-state-prover/src/service.rs Outdated
.unwrap()
.into();
let base_fee: U256 = parse_units(res.result.base_fee.clone(), "gwei")
.unwrap()
Copy link
Collaborator

@sveitser sveitser Oct 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Suggest removing unwraps, maybe add a helper function or From impl to parse the API response into a type that is convenient for us to work with and won't panic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should we just return error (propagate err up) or still use unwrap but hide inside some helper function?

jbearer
jbearer requested changes Oct 14, 2024
View reviewed changes
hotshot-state-prover/src/service.rs Outdated Show resolved Hide resolved
hotshot-state-prover/src/service.rs Outdated Show resolved Hide resolved
hotshot-state-prover/src/service.rs Outdated Show resolved Hide resolved
hotshot-state-prover/src/service.rs Outdated Show resolved Hide resolved
@alxiong
Copy link
Contributor Author

alxiong commented Oct 15, 2024 •
edited
Loading

I just found out that ethers-rs already has some code that fetch from etherscan price oralce. I'll try to use those instead of reinventing the wheel.

also we should use blocknative's feed (eg response) which is clearer and more detailed than etherscan

@alxiong alxiong changed the title fix: lower priority fee and correct PlonkVerifier in deployer.rs fix: lower priority fee Oct 15, 2024
@alxiong alxiong mentioned this pull request Oct 15, 2024
Merged
jbearer added a commit that referenced this pull request Oct 15, 2024
@jbearer
fix: use correct PlonkVerifier in deployer.rs (#2173)
7e16992 
separate the part of the changes originally included in #2148

### This PR:

- Switch to the right version of PlonkVerifier (in the deployer.rs we
incorrectly deployed PlonkVerifier2.sol which is secure but less
efficient)
@alxiong
Copy link
Contributor Author

alxiong commented Oct 15, 2024 •
edited
Loading

this is annoying: ethers-middleware define this field from BlockNative's feed to be u64, however, it's actually a float64 (e.g. sample, see the price field is a float).

Most likely Blocknative changed its feed since it has been 2 years since ethers-rs touch that code. It might work at the time of its writing.
Considering that ethers-rs are not actively maintained, I shall just copy the related code in our util crate and modify it so that it works, then when we move to alloy-rs, we can revisit and remove this code if unnecessary anymore.

@alxiong alxiong enabled auto-merge (squash) October 16, 2024 16:02
@alxiong alxiong disabled auto-merge October 16, 2024 23:39
@alysiahuggins
Copy link
Contributor

a nice to have would be a test for this added functionality

@alxiong
Copy link
Contributor Author

alxiong commented Oct 21, 2024 •
edited
Loading

@jbearer should I merge this or should I wait for #2189 to fix the slow test?

@alxiong alxiong enabled auto-merge (squash) October 21, 2024 15:47
@alxiong alxiong merged commit 711ab84 into main Oct 21, 2024
13 checks passed
@alxiong alxiong deleted the gambit-contract-patch branch October 21, 2024 16:25
@github-actions GitHub Actions
Copy link
Contributor

Backport failed for release-gambit, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin release-gambit
git worktree add -d .worktree/backport-2148-to-release-gambit origin/release-gambit
cd .worktree/backport-2148-to-release-gambit
git switch --create backport-2148-to-release-gambit
git cherry-pick -x 711ab84f811e814362ab97c1ba513e3d9055f8c8

alxiong added a commit that referenced this pull request Oct 22, 2024
@alxiong
fix: lower priority fee (#2148)
54e33bf 
* use (updated) blocknative oracle from ethers-rs

* log err if price oracle failed

(cherry picked from commit 711ab84)
@alxiong alxiong mentioned this pull request Oct 22, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alysiahuggins alysiahuggins alysiahuggins approved these changes

@jbearer jbearer jbearer approved these changes

@sveitser sveitser sveitser approved these changes

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

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

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

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

Assignees
No one assigned
Labels
backport release-gambit
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Set a lower priority fee for light client update
4 participants
@alxiong @alysiahuggins @sveitser @jbearer