-
Notifications
You must be signed in to change notification settings - Fork 0
Fallback Inbox contract changes #278
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
base: celo-integration-rebase-14.1
Are you sure you want to change the base?
Fallback Inbox contract changes #278
Conversation
b09bfab to
62d96d3
Compare
…e pass to the constructor.
… the inbox contract.
fcd5832 to
9af9291
Compare
| contract MockBatchAuthenticator { | ||
| mapping(bytes32 => bool) private validHashes; | ||
|
|
||
| function setValidBatchInfo(bytes32 hash, bool valid) external { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Similar to the comment above, setBatchValidity might be a better name.
| /// @title MockBatchAuthenticator | ||
| /// @notice Mock implementation for testing - only implements validBatchInfo | ||
| contract MockBatchAuthenticator { | ||
| mapping(bytes32 => bool) private validHashes; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: hashValidities might be a more accurate name since the mapped bool might be false.
| validHashes[hash] = valid; | ||
| } | ||
|
|
||
| function validBatchInfo(bytes32 hash) external view returns (bool) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Similar to the comment above, batchValidity might be a better name.
|
|
||
| function setUp() public virtual { | ||
| authenticator = new MockBatchAuthenticator(); | ||
| inbox = new BatchInbox(nonTeeBatcher, IBatchAuthenticator(address(authenticator)), deployer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will test_fallback_nonTeeBatcherDoesNotRequireAuth fail if we don't construct a BatchInbox with the nonTeeBatcher here, similar to test_fallback_unauthorizedAddressReverts? If so, do we have any setup for the teeBatcher? I guess there is, otherwise there's no way to distinguish authorized vs. unauthorized batchers, but I just don't find it in this file.
These are questions to check my understanding, not requesting any changes.
| if allocType == AllocTypeEspressoWithoutEnclave { | ||
| batcherPk, err := crypto.HexToECDSA(ESPRESSO_PRE_APPROVED_BATCHER_PRIVATE_KEY) | ||
| // Configure Espresso allocation types | ||
| if allocType == AllocTypeEspresso || allocType == AllocTypeEspressoWithoutEnclave || allocType == AllocTypeEspressoWithEnclave { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we have AllocTypeEspresso when we also have *WithEnclave and *WithoutEnclave?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if allocType == AllocTypeEspressoWithEnclave { | ||
| intent.Chains[0].EspressoEnabled = true | ||
| intent.Chains[0].NonTeeBatcher = crypto.PubkeyToAddress(batcherPk.PublicKey) | ||
| intent.Chains[0].TeeBatcher = crypto.PubkeyToAddress(batcherPk.PublicKey) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it for simplicity that we now set TeeBatcher even when allocType == AllocTypeEspressoWithoutEnclave? I think it's fine to set but not use it--just wanted to make sure it's intentional to put it in this condition clause.
Closes https://app.asana.com/1/1208976916964769/project/1209423958092927/task/1211849813289225?focus=true
This PR:
This PR does not:
Key places to review:
packages/contracts-bedrock/test/L1/BatchInbox.t.sol.How to test this PR:
Things tested
packages/contracts-bedrock/test/L1/BatchInbox.t.sol.