Make chain balance migration use totalWithdrawn/totalSuccessfulDeposits instead of totalSupply()#2041
Conversation
| function receiveMigrationOnL1(FinalizeL1DepositParams calldata _finalizeWithdrawalParams) external { | ||
| /// @notice Finalizes a token migration from L1 to Gateway. | ||
| /// @param _finalizeWithdrawalParams Message inclusion proof parameters. | ||
| function receiveL1ToGatewayMigrationOnL1(FinalizeL1DepositParams calldata _finalizeWithdrawalParams) external { |
There was a problem hiding this comment.
I would do this via a bool, it would remove quite a bit of code duplication.
There was a problem hiding this comment.
I dont like bool because the fields for the structs are different, but at least some of the duplication can be moved to the inner functions I think
There was a problem hiding this comment.
I look at the code and it seems that the amount of common logic is not that large (asset id check, etc).
but the rest of the operations while they call the same functions, they call them with parameters with different means and so on, so IMO keeping those separate introduces less mental load for the auditor, though indeed a bit more code is needed
StanislavBreadless
force-pushed
the
sb/light-chain-balance-migration
branch
from
acf08fd to
4044f76
Compare
StanislavBreadless
force-pushed
the
sb/light-chain-balance-migration
branch
from
4044f76 to
b1fe69e
Compare
l1-contracts/contracts/bridge/asset-tracker/IL1AssetTracker.sol
Outdated
Show resolved
Hide resolved
| /// before v31 it is equal to `2^256-1 - <sum of other chainBalances, including l1>`. For new tokens it is exactly `2^256-1`. | ||
| /// @param totalDepositedFromL1 Total amount deposited from L1 to the chain since v31 accounting started. Note, that it is not | ||
| /// just about any L1->L2 deposit, but only those that debited the chainBalance on L1 directly and it is assumed that every such | ||
| /// deposit will be processed while the chain is still settling on L1. It is the responsibility of the chain admin to ensure that. |
There was a problem hiding this comment.
need to double check whether it is the responsibility of chain admin or the chain, i.e. does our impl enforce it in any way
| function receiveMigrationOnL1(FinalizeL1DepositParams calldata _finalizeWithdrawalParams) external { | ||
| /// @notice Finalizes a token migration from L1 to Gateway. | ||
| /// @param _finalizeWithdrawalParams Message inclusion proof parameters. | ||
| function receiveL1ToGatewayMigrationOnL1(FinalizeL1DepositParams calldata _finalizeWithdrawalParams) external { |
There was a problem hiding this comment.
I look at the code and it seems that the amount of common logic is not that large (asset id check, etc).
but the rest of the operations while they call the same functions, they call them with parameters with different means and so on, so IMO keeping those separate introduces less mental load for the auditor, though indeed a bit more code is needed
StanislavBreadless
requested review from
Deniallugo and
vladbochok
as code owners
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e929e2cf0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| chainBalance[_chainId][_assetId] = 0; | ||
| assetMigrationNumber[_chainId][_assetId] = chainMigrationNumber; |
There was a problem hiding this comment.
This eagerly zeroes chainBalance and advances assetMigrationNumber before L1 has accepted the Gateway->L1 migration message. If L1AssetTracker.receiveGatewayToL1MigrationOnL1 later reverts (for example on its settlement-layer whitelist or migration-number checks), the same migration cannot be retried because initiateGatewayToL1MigrationOnGateway requires assetMigrationNumber < chainMigrationNumber, so the balance is effectively stranded on Gateway.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
it is assumed it wont happen
kelemeno
left a comment
kelemeno
left a comment
There was a problem hiding this comment.
ok, I reviewed the main processes now, looks good
Co-authored-by: 0xValera <55665573+0xValera@users.noreply.github.com>
…' into sb/light-chain-balance-migration
Co-authored-by: 0xValera <55665573+0xValera@users.noreply.github.com>
…' into sb/light-chain-balance-migration
|
Coverage after merging sb/light-chain-balance-migration into draft-v31 will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
4 participants
What ❔
Why ❔
Checklist