Rename TEE Batcher#395
Conversation
|
Claude finished @jjeangal's task in 3m 8s —— View job Review: PR #395 — Rename TEE Batcher
SummaryClean renaming PR that consistently replaces Issues FoundABI/Storage Layout Snapshots Not Updated (Medium) The snapshot files still contain the old names and were not updated in this PR:
These snapshots need to be regenerated to reflect the contract changes. This could cause CI snapshot checks to fail or create confusion for anyone relying on the ABI snapshots.
Minor Inconsistencies in Comments (Nit) Two places use "non-Espresso batcher" instead of the preferred "fallback batcher" — see inline comments on What Looks Good
|
|
|
||
| // TestBatcherSwitching tests that the batcher can be switched from the TEE-enabled | ||
| // batcher to a fallback non-TEE batcher using the BatchAuthenticator contract. | ||
| // batcher to a fallback non-Espresso batcher using the BatchAuthenticator contract. |
There was a problem hiding this comment.
Nit: The rest of the PR uses "fallback batcher" consistently, but this comment says "non-Espresso batcher". Consider updating for consistency:
| // batcher to a fallback non-Espresso batcher using the BatchAuthenticator contract. | |
| // batcher to a fallback batcher using the BatchAuthenticator contract. |
| require.NoError(t, err) | ||
|
|
||
| t.Logf("TEE batcher published: %v, non-TEE batcher published: %v", teePublished, nonTeePublished) | ||
| t.Logf("Espresso batcher published: %v, non-Espresso batcher published: %v", espressoPublished, fallbackPublished) |
There was a problem hiding this comment.
Nit: Same inconsistency here — the log message says "non-Espresso batcher" while the variable is named fallbackPublished. Consider aligning:
| t.Logf("Espresso batcher published: %v, non-Espresso batcher published: %v", espressoPublished, fallbackPublished) | |
| t.Logf("Espresso batcher published: %v, fallback batcher published: %v", espressoPublished, fallbackPublished) |
| } | ||
|
|
||
| function registerSigner(bytes calldata attestationTbs, bytes calldata signature) external { | ||
| function registerSigner(bytes calldata verificationData, bytes calldata data) external { |
There was a problem hiding this comment.
The new parameter name data is quite generic. The underlying registerService in the TEE verifier mocks uses attestation for the first parameter. Consider whether verificationData and data are descriptive enough, or if something like attestation and proof (or attestationData and proofData) would better communicate the purpose of each parameter.
Not a blocker, but worth considering for developer clarity.
There was a problem hiding this comment.
Code Review
This pull request renames 'TEE' and 'non-TEE' batcher references to 'Espresso' and 'fallback' across the system's contracts, bindings, and tests. It also adds the 'rootClaimByChainId' function to the fault dispute game bindings. Feedback was provided to improve consistency by renaming the 'activeIsTee' variable and related event parameters in the 'BatchAuthenticator' contract to match the new naming convention, despite the potential for breaking changes in the ABI.
|
|
||
| /// @notice Flag indicating which batcher is currently active. | ||
| /// @dev When true the TEE batcher is active; when false the non-TEE batcher is active. | ||
| /// @dev When true the Espresso batcher is active; when false the fallback batcher is active. |
There was a problem hiding this comment.
The comment has been updated to refer to the "Espresso batcher" and "fallback batcher", but the variable activeIsTee (on line 38) and the event BatcherSwitched (referenced in switchBatcher) still use the "Tee" terminology. For consistency with the PR's objective of renaming TEE and non-TEE batchers, consider renaming activeIsTee to activeIsEspresso and updating the associated event and functions. Note that this would be a breaking change for the contract ABI, similar to the renaming of teeBatcher to espressoBatcher.
shenkeyao
left a comment
There was a problem hiding this comment.
Thank you for doing the renaming!
I realized that we should do a similar change in the tee-op-deploy repo as well, so I just created this ticket: https://app.asana.com/1/1208976916964769/project/1209392461754458/task/1213920204955146?focus=true. I can take on it next week.
|
Here is the error: |
Sorry it was a problem on my end. Test is passing now. |
Maybe it was related to the code not using the updated |
No, I think it was an issue with some local docker configuration. |
* Rename tee batcher * change name `activeIsTee` * update function selector for espresso batcher * update security analysis doc * delete unrelated script * espresso devnet tests use fallback & espresso terms * modify .env var names * (hopefully last references) * push new batch authenticator bindings * update espresso-streamers commit * pin to point to espresso-streamers last PR
This PR
teeBatcher/activeIsTee→espressoBatcher/activeIsEspressothroughoutBatchAuthenticator.soland its interface, generated Go bindings, batcher logic, and tests — aligns naming with Espresso-specific semantics rather than implementation detail (TEE).NON_TEEdevnet Docker Compose profile toFALLBACKand updates all test references to useespresso/fallbackvariable naming instead oftee/nonTee.registerSignerparameters inBatchAuthenticator.solfromattestationTbs/signaturetoverificationData/datafor a more generic interface..envvarsTEE_BATCHER_ADDRESS/TEE_BATCHER_PRIVATE_KEY→ESPRESSO_BATCHER_ADDRESS/ESPRESSO_BATCHER_PRIVATE_KEY.espresso/SECURITY_ANALYSIS.mdto consistently say "Espresso batcher" and "fallback batcher" in place of "TEE batcher" and "non-TEE batcher".Key Places to Review
espressoBatcher, flagactiveIsEspresso, settersetEspressoBatcher, eventEspressoBatcherUpdated; alsoregisterSignerparam rename (see bullets 1 & 3)EspressoBatcher,ActiveIsEspresso,SetEspressoBatcher) are consistent with call sites (see bullet 1)isBatcherActivenow callsActiveIsEspresso; logic unchanged, only naming updated (see bullet 1)NON_TEE→FALLBACKprofile,activeIsTee→activeIsEspressovariable (see bullet 2)How to Test This PR
Asana tasks