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

Arbitrum/atlas v1.1 #445

Open
wants to merge 103 commits into
base: arbitrum/atlas-v1.0
Choose a base branch
from
Open

Arbitrum/atlas v1.1 #445

wants to merge 103 commits into from

Conversation

BenSparksCode
Copy link
Contributor

@BenSparksCode BenSparksCode commented Oct 24, 2024 •
edited
Loading

Changes:

  • Brings Atlas v1.1 updates as detailed in feat: Atlas v1.1 #428 to our custom Arbitrum version of Atlas
  • Brings size cuts as detailed in refactor: contract size cuts #444 to our custom Arbitrum version of Atlas
  • Minor tweaks to Atlas and L2 Gas Calculator deploy scripts to be more generic across all chains
  • Detect if chain is Arbitrum Nova in the constructor of ArbitrumGasCalculator.

Atlas contract size is 24,424 bytes (152 bytes below limit) and is thus deployable on Arbitrum.

BenSparksCode and others added 30 commits September 4, 2024 17:28
@BenSparksCode
fix: conditionally forward returnData to solvers
a0dc8ff
@BenSparksCode
fix: enforce userOp.gas as userOp's gas limit
b46981c
@BenSparksCode
fix: use smaller of userOp.gas and gasleft() as limit
5e05707
@BenSparksCode
chore: update Atlas license
4c9c5e3
@BenSparksCode
chore: update Atlas license
a448c98
@jj1980a
base L2 gas calculator contract
cf271a6
@jj1980a
contract ownable
f5e9dd1
@jj1980a
introduce a customizable offset value to be added to passed call data…
f8dbebc 
… length
@jj1980a
deploy script
fd33c38
@jj1980a
forge update
2448be0
@thogard785
back-end approach
8fcf58f
@BenSparksCode
Merge pull request #421 from FastLane-Labs/license-fix
58fa483 
chore: update Atlas license
@BenSparksCode
Merge branch 'main' into atlas-v1.1
237be45
@BenSparksCode
Merge branch 'atlas-v1.1' into solver-surcharge-tracking
6da7e88
@BenSparksCode
fmt: forge fmt
47bcfd8
@BenSparksCode
fix: TrebleSwap tests to handle new surcharge rules
44eb8e0
@BenSparksCode
fix: record accurate solverSurcharge figure
392ba0e
@BenSparksCode
fix: 2 Escrow tests - metacall no longer reverts
0f749da
@BenSparksCode
fix: FLOnline breaking tests for surcharge rule change
6f1f469
@BenSparksCode
chore: Base L2 Gas Calculator deploy script tweaks
c4de266
@jj1980a @BenSparksCode
Apply suggestions from code review
20ac0af 
Co-authored-by: Ben Sparks <[email protected]>
@jj1980a
forge fmt
04493e7
@jj1980a
fix deploy script
bc6f32a
@BenSparksCode
feat: add easier way to control hook failure for tests
547588b
@BenSparksCode
test: add forwardReturnData config tests
5605e76
@BenSparksCode
test: use hookShouldRevert pattern in Escrow.t.sol
4aa7646
@BenSparksCode
chore: rel import paths in DAppControl and parents
6109348
@BenSparksCode
chore: switch to relative imports
39394d9
@BenSparksCode
chore: use relative imports in BaseTest
705e15e
@BenSparksCode
chore: use relative imports in TestAtlas
5a25060
@BenSparksCode BenSparksCode marked this pull request as ready for review October 24, 2024 13:20
@BenSparksCode BenSparksCode mentioned this pull request Oct 24, 2024
Draft
0x1NotMe
0x1NotMe approved these changes Oct 25, 2024
View reviewed changes
Copy link
Contributor

@0x1NotMe 0x1NotMe left a comment

Choose a reason for hiding this comment

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

LGTM.

The changes in the AtlasConstants.sol I assume are intentional?
_LOCK_PHASE_MASK from bytes32 to uint256

@BenSparksCode
Copy link
Contributor Author

The changes in the AtlasConstants.sol I assume are intentional? _LOCK_PHASE_MASK from bytes32 to uint256

Yes. Before we had to convert everything to/from bytes32 to use transient storage. Now with the transient keyword it will handle those conversions to our variable types, so using uint256 here cuts out 1 manual conversion step in our source code every time we use the variable.

thogard785
thogard785 reviewed Oct 25, 2024
View reviewed changes
Copy link
Contributor

@thogard785 thogard785 left a comment

Choose a reason for hiding this comment

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

lgtm - just one minor thing i noticed.

src/contracts/atlas/Escrow.sol Outdated
@@ -92,12 +102,13 @@ abstract contract Escrow is AtlETH {
{
bool _success;
bytes memory _data;
uint256 _gasLimit = userOp.gas > gasleft() ? gasleft() : userOp.gas;
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't there be an offset to allow graceful returns if we run out of gas? plus the issue about the call forcing 63/64? We may want to do gasleft() * 63 / 64 - gracefulReturnOffset', where gracefulReturnOffset` is enough to store what we need and return the correct atlas-formatted error. (50k?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, good call, will add 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.

PR here: #446

Will merge that into Atlas v1.1 and then update this Arbitrum version with the changes if PR is accepted.

BenSparksCode and others added 18 commits October 28, 2024 07:50
@BenSparksCode
Merge pull request #444 from FastLane-Labs/size-cuts
d24c7ae 
refactor: contract size cuts
@BenSparksCode
fix: apply graceful return gas offset to userOp call
47c3b17
@BenSparksCode
test: userOp out-of-gas graceful return case
17340c8
@BenSparksCode
Merge pull request #446 from FastLane-Labs/userOp-gas-limit-fix
385ae48 
fix: userOp graceful return gas offset
@BenSparksCode
docs: version is Atlas v1.1 in natspec
3a4972d
@BenSparksCode
Merge branch 'atlas-v1.1' into arbitrum/atlas-v1.1
7fd3119
@BenSparksCode
feat: add bidAmount to SolverTxResult event
17e81f2
@BenSparksCode
feat: add bidToken to SolverTxResult event
8a475a1
@BenSparksCode
Merge pull request #450 from FastLane-Labs/solver-event-analytics
64be23c 
feat: add `bidAmount` to `SolverTxResult` event
@BenSparksCode
Merge branch 'atlas-v1.1' into arbitrum/atlas-v1.1
0ae966f
@jj1980a
add indexed dapp control address in solver tx result event
6df621f
@BenSparksCode
fix: rearrange event item order - solves stack too deep
617e8e6
@BenSparksCode
Merge pull request #451 from FastLane-Labs/solver-event
f837859 
feat: add indexed dapp control address in solver tx result event
@BenSparksCode
Merge branch 'atlas-v1.1' into arbitrum/atlas-v1.1
6642910
@BenSparksCode
feat: add surcharge settings to deploy script
4807453
@BenSparksCode
chore: deploy Atlas v1.1 on Polygon
60a70b3
@BenSparksCode
Merge pull request #452 from FastLane-Labs/surcharge-settings-deploy
e6b1718 
feat: add surcharge settings to deploy script
@BenSparksCode
Merge branch 'atlas-v1.1' into arbitrum/atlas-v1.1
5a7baa8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thogard785 thogard785 thogard785 left review comments

@0x1NotMe 0x1NotMe 0x1NotMe approved these changes

@aanand1 aanand1 Awaiting requested review from aanand1

@jj1980a jj1980a Awaiting requested review from jj1980a

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@BenSparksCode @0x1NotMe @thogard785 @jj1980a