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

Op Farms #1

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

Op Farms #1

wants to merge 35 commits into from

Conversation

oldchili
Copy link
Collaborator

@oldchili oldchili commented Aug 5, 2024

No description provided.

deploy/L2FarmProxySpell.sol Outdated Show resolved Hide resolved
deploy/FarmProxyInit.sol Outdated Show resolved Hide resolved
script/Init.s.sol Outdated Show resolved Hide resolved
script/Init.s.sol Outdated Show resolved Hide resolved
@DaiFoundation-DevOps
Copy link

DaiFoundation-DevOps commented Aug 20, 2024

CLA assistant check
All committers have signed the CLA.

oldchili and others added 2 commits August 21, 2024 17:54
src/L1FarmProxy.sol Outdated Show resolved Hide resolved
assertEq(dss.chainlog.getAddress("FARM_PROXY_TKA_TKB_BASE"), address(l1Proxy));
assertEq(dss.chainlog.getAddress("REWARDS_DIST_TKA_TKB_BASE"), cfg.vestedRewardsDistribution);

l2Domain.relayFromHost(true);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just consider this relayFromHost would also be executing the L2 side of the spell for the Bridge set up. Nothing really wrong with that but doesn't get isolated in the setupGateways function as was probably the intention.

Copy link
Collaborator Author

@oldchili oldchili Aug 25, 2024

Choose a reason for hiding this comment

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

This code is the same as in arbitrum-farms.
For some reason when adding l2Domain.relayFromHost(false); at the end of setupGateways the test fails with a message of "revert: CrossDomainMessenger: message has already been relayed".
I think @telome saw something similar, but not sure.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok yeah let's leave this open for now. It doesn't affect the audit as not in the scope anyway.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Still not sure what exactly is the root cause of the issue but there seems to be some bug in foundry that causes the exit from setUp() to incorrectly restore the storage of OptimismDomain. For now a simple workaround could be to avoid using setUp(). For example, renaming setUp() to setUpFarm() and calling setUpFarm() at the beginning of distribute() avoids the issue.

.env.example Outdated Show resolved Hide resolved
script/Init.s.sol Outdated Show resolved Hide resolved
.env.example Outdated Show resolved Hide resolved
.env.example Outdated Show resolved Hide resolved
Copy link
Collaborator

@telome telome left a comment

Choose a reason for hiding this comment

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

Good to send to audit (minor note that op-token-bridge can be updated again to its most recent commit).

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.

Looks good

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