Skip to content

Arbitrum/atlas v1.0#439

Draft
0x1NotMe wants to merge 18 commits intomainfrom
arbitrum/atlas-v1.0
Draft

Arbitrum/atlas v1.0#439
0x1NotMe wants to merge 18 commits intomainfrom
arbitrum/atlas-v1.0

Conversation

@0x1NotMe
Copy link
Contributor

@0x1NotMe 0x1NotMe commented Oct 4, 2024

This change introduces a new library SafeBlockNumber

  • it will detect the chainId for Arbitrum chains and use the ArbSys precompile to return the Arbitrum Block

s_balanceOf[owner].unbonding += _amt;

emit Unbond(owner, amount, block.number + ESCROW_DURATION + 1);
emit Unbond(owner, amount, SafeBlockNumber.get() + ESCROW_DURATION + 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably cache the block number in a local var if used more than once in a function. Looks like thats done in the other files but not here. Any reason not to here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will work on a PR for the transient storage idea which and add a caches here as well

@BenSparksCode
Copy link
Contributor

BenSparksCode commented Oct 23, 2024

Seeing as this call to the Arbitrum ArbSys contract happens every time we need to use the block.number in Atlas, we could probably save a lot of gas and contract size by instead using a transient variable called t_blockNumber and update it at the start of the metacall, then just read that var when we need the block number.

I think we'd have to change it from a library to an internal function, maybe in Storage.sol, because libraries can't contain state vars, but that should be okay.

We would keep the SafeBlockNumber.get() library for reuse in AtlasVerification (stays as is), and in the AtlETH functions in Atlas (stays as is) because those would usually be called outside of a metacall. But otherwise, we load the block number into t_blockNumber at start, and for GasAccounting and Escrow usage, we just read the transient var.

Wdyt?

EDIT: This isn't urgent. In its current state, Atlas is a deployable size (as of #445), and the gas diffs shouldn't be significant on L2s like Arbitrum.

@BenSparksCode
Copy link
Contributor

I think this PR + changes in #445 are good for an official Arbitrum release.

We should probably keep this version living on a separate parallel branch to main which holds the version of Atlas deployable on all other chains, as I think this would increase gas costs for other chains, even though it should technically be compatible.

The one piece from this PR + #445 we should bring back to main is the updated deploy script, as it handles conditional deployment of an L2 Gas Calculator better than before, and non Arbitrum L2 deployments will benefit from that.

@0x1NotMe
Copy link
Contributor Author

I think we'd have to change it from a library to an internal function, maybe in Storage.sol, because libraries can't contain state vars, but that should be okay.

I really like this idea because we accessing the block number quite a few time during accounting stage

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.

2 participants

Comments