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.

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

Add L1 & L2 Farm Proxies #1

Open
wants to merge 58 commits into
base: master
Choose a base branch
from
Open

Add L1 & L2 Farm Proxies #1

wants to merge 58 commits into from

Conversation

telome
Copy link
Collaborator

@telome telome commented May 22, 2024

No description provided.

src/L1FarmProxy.sol Outdated Show resolved Hide resolved
src/L1FarmProxy.sol Outdated Show resolved Hide resolved
src/L1FarmProxy.sol Outdated Show resolved Hide resolved
src/L1FarmProxy.sol Outdated Show resolved Hide resolved
test/Integration.t.sol Outdated Show resolved Hide resolved
Copy link
Collaborator

@oldchili oldchili left a comment

Choose a reason for hiding this comment

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

Added some more comment.
However, feel free to delay handling until we finalize the architecture (like if we want to send the 2nd xchain message or not).

deploy/FarmProxyInit.sol Outdated Show resolved Hide resolved
deploy/FarmProxyInit.sol Outdated Show resolved Hide resolved
deploy/FarmProxyInit.sol Outdated Show resolved Hide resolved
deploy/FarmProxyInit.sol Outdated Show resolved Hide resolved
deploy/FarmProxyInit.sol Show resolved Hide resolved
deploy/FarmProxyInit.sol Show resolved Hide resolved
deploy/L2FarmProxySpell.sol Outdated Show resolved Hide resolved
deploy/L2FarmProxySpell.sol Outdated Show resolved Hide resolved
deploy/L2FarmProxySpell.sol Show resolved Hide resolved
src/L1FarmProxy.sol Show resolved Hide resolved
src/L1FarmProxy.sol Show resolved Hide resolved
test/L1FarmProxy.t.sol Outdated Show resolved Hide resolved
test/L1FarmProxy.t.sol Outdated Show resolved Hide resolved
test/L1FarmProxy.t.sol Outdated Show resolved Hide resolved
test/L1FarmProxy.t.sol Show resolved Hide resolved
test/L2FarmProxy.t.sol Outdated Show resolved Hide resolved
deploy/FarmProxyInit.sol Outdated Show resolved Hide resolved
deploy/FarmProxyInit.sol Outdated Show resolved Hide resolved
src/L1FarmProxy.sol Outdated Show resolved Hide resolved
contract Estimate is Script {
using stdJson for string;

uint256 constant MAX_L1_BASE_FEE_ESTIMATE = 1 gwei; // worst-case estimate for l1BaseFeeEstimate (representing the blob base fee) returned from https://github.com/OffchainLabs/nitro-contracts/blob/90037b996509312ef1addb3f9352457b8a99d6a6/src/node-interface/NodeInterface.sol#L95
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we have any reference for the 1 gwei max value?

Copy link
Collaborator Author

@telome telome Jul 11, 2024

Choose a reason for hiding this comment

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

That's just a rough worst-case estimate, which is very conservative. I was looking at the blob fee chart on https://ultrasound.money/#blobs and it seemed that the fee would very rarely spike beyond 1 gwei in the last few months.

A more formal method could be to use the baseFeePerBlobGas returned from the eth_feeHistory rpc command and calculate e.g. the 99th percentile for that value. Maybe this could be done in another script.

Copy link
Collaborator

@oldchili oldchili Jul 11, 2024

Choose a reason for hiding this comment

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

I guess I don't understand how it works.
Do we know the meaning of l1BaseFeeEstimate? I thought it was the regular l1 base fee and not related to blobs.
But then you say MAX_L1_BASE_FEE_ESTIMATE if related to blobs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Originally l1BaseFeeEstimate was an estimate of the L1 base fee but then after they started using blobs it became an estimate of the blob fee. They didn't update their doc, changed their interfaces or renamed any variable, so it's indeed a bit confusing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see, so how did you figure that out?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Some Arbitrum discord messages were alluding to it, like this one image
and then if you check the actual values returned for l1BaseFeeEstimate, you'll see you consistently get values lower than 1 gwei.

Though there is still a difference between the l1BaseFeeEstimate value and baseFeePerBlobGas so not sure if Arbitrum adds some buffer on top of baseFeePerBlobGas. Probably something to check with them.

Copy link
Collaborator

@oldchili oldchili left a comment

Choose a reason for hiding this comment

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

LGTM to send o audit

Copy link
Collaborator

@sunbreak1211 sunbreak1211 left a comment

Choose a reason for hiding this comment

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

LGTM

@DaiFoundation-DevOps
Copy link

DaiFoundation-DevOps commented Aug 22, 2024

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ sunbreak1211
❌ telome


telome seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

telome and others added 3 commits August 23, 2024 09:23
Co-authored-by: telome <>
* Update deps

* Update arb bridge

---------

Co-authored-by: telome <>
Copy link
Collaborator

@oldchili oldchili left a comment

Choose a reason for hiding this comment

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

LGTM

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.

4 participants