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

batcher allowlist #450

Merged
merged 1 commit into from
Apr 5, 2024
Merged

batcher allowlist #450

merged 1 commit into from
Apr 5, 2024

Conversation

0x0aa0
Copy link
Contributor

@0x0aa0 0x0aa0 commented Apr 5, 2024

Why are these changes needed?

We need to allowlist a fallback batcher

Checks

  • I've made sure the lint is passing in this PR.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, in that case, please comment that they are not relevant.
  • Testing Strategy
    • Unit tests
    • Integration tests
    • This PR is not tested :(

@0x0aa0 0x0aa0 requested review from gpsanant and ChaoticWalrus April 5, 2024 18:40
Comment on lines -56 to +57
/// @notice address that is permissioned to confirm batches
address public batchConfirmer;
/// @notice mapping of addressed that are permissioned to confirm batches
mapping(address => bool) public isBatchConfirmer;

Choose a reason for hiding this comment

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

this is changing in the same storage slot. will this break testnet deployment when upgrading?
I guess when upgrading we can just:

  1. change the confirmer to the zero address
  2. upgrade
  3. set batch confirmer

Comment on lines +133 to +134
isBatchConfirmer[_batchConfirmer] = !isBatchConfirmer[_batchConfirmer];
emit BatchConfirmerStatusChanged(_batchConfirmer, isBatchConfirmer[_batchConfirmer]);

Choose a reason for hiding this comment

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

I'm not a huge fan of this pattern, but it's OK with me

Copy link

@ChaoticWalrus ChaoticWalrus left a comment

Choose a reason for hiding this comment

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

Approving this small change.
Adding test coverage for new behavior would be good, but I understand that's not the priority right now.

Noting this one storage slot change which may be relevant for testnet https://github.com/Layr-Labs/eigenda/pull/450/files#r1554183962

@0x0aa0 0x0aa0 merged commit 78b5f16 into mainnet-deploy Apr 5, 2024
3 checks passed
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