Encode the success boolean (alongised the returndata) for calls made in the EXEC_TYPE_TRY mode#6420
Encode the success boolean (alongised the returndata) for calls made in the EXEC_TYPE_TRY mode#6420Amxx wants to merge 1 commit intoOpenZeppelin:masterfrom
Conversation
…in the EXEC_TYPE_TRY mode
|
WalkthroughThis pull request modifies the 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
test/account/utils/draft-ERC7579Utils.test.js (1)
91-104: Consider adding test coverage for successfulEXEC_TYPE_TRYexecutions.The tests only verify the return format for TRY mode when calls fail. It would be valuable to add tests confirming that successful calls in TRY mode also return
abi.encode(true, returndata)with the correct success boolean.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/account/utils/draft-ERC7579Utils.test.js` around lines 91 - 104, Add a test that mirrors the failing TRY-case but for a successful call: call utils.$execSingle with EXEC_TYPE_TRY using encodeSingle(this.target, value, this.target.interface.encodeFunctionData('mockFunction')) (or another non-reverting function on CallReceiverMock), then assert it does NOT emit 'ERC7579TryExecuteFail' and DOES emit 'return$execSingle' withArgs([coder.encode(['bool','bytes'], [true, returndata])]) where returndata is the raw bytes returned by the mock function (obtain via this.target.interface.encodeFunctionResult or by calling the function locally to capture its return data); use the same EXEC_TYPE_TRY, $execSingle, and return$execSingle symbols as in the existing test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/account/utils/draft-ERC7579Utils.test.js`:
- Around line 248-252: The test uses the semantic constant CALL_TYPE_CALL where
the delegate-call result should assert on the numeric index 0; update the
assertion to use the explicit index 0 instead of CALL_TYPE_CALL in the .withArgs
for ERC7579TryExecuteFail and/or return$execDelegateCall so the expected event
payload matches the delegate-call-at-index-0 shape (keep other values:
encodeErrorString('CallReceiverMock: reverting') and
coder.encode(['bool','bytes'], [false, encodeErrorString('CallReceiverMock:
reverting')]) unchanged). Locate the failing assertion around
this.utils.$execDelegateCall(...) that references CALL_TYPE_CALL and replace it
with 0.
- Around line 99-103: The test is using CALL_TYPE_CALL as the first arg to
ERC7579TryExecuteFail (which is actually the uint256 batchExecutionIndex)
causing a semantic mismatch; update the assertion in
test/account/utils/draft-ERC7579Utils.test.js that calls this.utils.$execSingle
so the ERC7579TryExecuteFail expectation uses the explicit numeric index (e.g.,
0) instead of CALL_TYPE_CALL, i.e. change withArgs(CALL_TYPE_CALL, ...) to
withArgs(0, ...), leaving the rest of the expectation (encodeErrorString payload
and the return$execSingle assertion) unchanged.
- Around line 183-193: The test asserts the wrong event argument by using
CALL_TYPE_BATCH (which equals '0x01') instead of the explicit numeric index for
the failing item; update the .withArgs for the ERC7579TryExecuteFail event to
use the explicit index 1 (the second batch item) rather than CALL_TYPE_BATCH so
the expected event matches the actual failing call in this.utils.$execBatch
invoked with EXEC_TYPE_TRY; keep the rest of the .withArgs checks for
return$execBatch as-is and ensure you change only the event index reference
(ERC7579TryExecuteFail) to the literal 1.
---
Nitpick comments:
In `@test/account/utils/draft-ERC7579Utils.test.js`:
- Around line 91-104: Add a test that mirrors the failing TRY-case but for a
successful call: call utils.$execSingle with EXEC_TYPE_TRY using
encodeSingle(this.target, value,
this.target.interface.encodeFunctionData('mockFunction')) (or another
non-reverting function on CallReceiverMock), then assert it does NOT emit
'ERC7579TryExecuteFail' and DOES emit 'return$execSingle'
withArgs([coder.encode(['bool','bytes'], [true, returndata])]) where returndata
is the raw bytes returned by the mock function (obtain via
this.target.interface.encodeFunctionResult or by calling the function locally to
capture its return data); use the same EXEC_TYPE_TRY, $execSingle, and
return$execSingle symbols as in the existing test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f0fb6692-2336-48e7-be80-12a70814d2fb
📒 Files selected for processing (2)
contracts/account/utils/draft-ERC7579Utils.soltest/account/utils/draft-ERC7579Utils.test.js
| await expect(this.utils.$execSingle(data, EXEC_TYPE_TRY)) | ||
| .to.emit(this.utils, 'ERC7579TryExecuteFail') | ||
| .withArgs( | ||
| CALL_TYPE_CALL, | ||
| ethers.solidityPacked( | ||
| ['bytes4', 'bytes'], | ||
| [selector('Error(string)'), coder.encode(['string'], ['CallReceiverMock: reverting'])], | ||
| ), | ||
| ); | ||
| .withArgs(CALL_TYPE_CALL, encodeErrorString('CallReceiverMock: reverting')) | ||
| .to.emit(this.utils, 'return$execSingle') | ||
| .withArgs([coder.encode(['bool', 'bytes'], [false, encodeErrorString('CallReceiverMock: reverting')])]); |
There was a problem hiding this comment.
Use explicit numeric index instead of CALL_TYPE_CALL for batchExecutionIndex.
The ERC7579TryExecuteFail event's first parameter is uint256 batchExecutionIndex, but the test uses CALL_TYPE_CALL (a call type constant). This works only because CALL_TYPE_CALL = '0x00' happens to equal 0, which is the correct index for a single execution.
Using call type constants as indices is semantically misleading and could cause confusion or bugs during refactoring.
Suggested fix
await expect(this.utils.$execSingle(data, EXEC_TYPE_TRY))
.to.emit(this.utils, 'ERC7579TryExecuteFail')
- .withArgs(CALL_TYPE_CALL, encodeErrorString('CallReceiverMock: reverting'))
+ .withArgs(0, encodeErrorString('CallReceiverMock: reverting'))
.to.emit(this.utils, 'return$execSingle')
.withArgs([coder.encode(['bool', 'bytes'], [false, encodeErrorString('CallReceiverMock: reverting')])]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await expect(this.utils.$execSingle(data, EXEC_TYPE_TRY)) | |
| .to.emit(this.utils, 'ERC7579TryExecuteFail') | |
| .withArgs( | |
| CALL_TYPE_CALL, | |
| ethers.solidityPacked( | |
| ['bytes4', 'bytes'], | |
| [selector('Error(string)'), coder.encode(['string'], ['CallReceiverMock: reverting'])], | |
| ), | |
| ); | |
| .withArgs(CALL_TYPE_CALL, encodeErrorString('CallReceiverMock: reverting')) | |
| .to.emit(this.utils, 'return$execSingle') | |
| .withArgs([coder.encode(['bool', 'bytes'], [false, encodeErrorString('CallReceiverMock: reverting')])]); | |
| await expect(this.utils.$execSingle(data, EXEC_TYPE_TRY)) | |
| .to.emit(this.utils, 'ERC7579TryExecuteFail') | |
| .withArgs(0, encodeErrorString('CallReceiverMock: reverting')) | |
| .to.emit(this.utils, 'return$execSingle') | |
| .withArgs([coder.encode(['bool', 'bytes'], [false, encodeErrorString('CallReceiverMock: reverting')])]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/account/utils/draft-ERC7579Utils.test.js` around lines 99 - 103, The
test is using CALL_TYPE_CALL as the first arg to ERC7579TryExecuteFail (which is
actually the uint256 batchExecutionIndex) causing a semantic mismatch; update
the assertion in test/account/utils/draft-ERC7579Utils.test.js that calls
this.utils.$execSingle so the ERC7579TryExecuteFail expectation uses the
explicit numeric index (e.g., 0) instead of CALL_TYPE_CALL, i.e. change
withArgs(CALL_TYPE_CALL, ...) to withArgs(0, ...), leaving the rest of the
expectation (encodeErrorString payload and the return$execSingle assertion)
unchanged.
| await expect(this.utils.$execBatch(data, EXEC_TYPE_TRY)) | ||
| .to.emit(this.utils, 'ERC7579TryExecuteFail') | ||
| .withArgs( | ||
| CALL_TYPE_BATCH, | ||
| ethers.solidityPacked( | ||
| ['bytes4', 'bytes'], | ||
| [selector('Error(string)'), coder.encode(['string'], ['CallReceiverMock: reverting'])], | ||
| .withArgs(CALL_TYPE_BATCH, encodeErrorString('CallReceiverMock: reverting')) | ||
| .to.emit(this.utils, 'return$execBatch') | ||
| .withArgs([ | ||
| coder.encode( | ||
| ['bool', 'bytes'], | ||
| [true, this.target.interface.encodeFunctionResult('mockFunction', ['0x1234'])], | ||
| ), | ||
| ); | ||
| coder.encode(['bool', 'bytes'], [false, encodeErrorString('CallReceiverMock: reverting')]), | ||
| ]); |
There was a problem hiding this comment.
Same issue: use explicit index 1 instead of CALL_TYPE_BATCH.
The failing call is the second item (index 1), and CALL_TYPE_BATCH = '0x01' coincidentally equals 1. The return value assertions correctly validate the new abi.encode(success, returndata) format for each batch item.
Suggested fix
await expect(this.utils.$execBatch(data, EXEC_TYPE_TRY))
.to.emit(this.utils, 'ERC7579TryExecuteFail')
- .withArgs(CALL_TYPE_BATCH, encodeErrorString('CallReceiverMock: reverting'))
+ .withArgs(1, encodeErrorString('CallReceiverMock: reverting'))
.to.emit(this.utils, 'return$execBatch')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await expect(this.utils.$execBatch(data, EXEC_TYPE_TRY)) | |
| .to.emit(this.utils, 'ERC7579TryExecuteFail') | |
| .withArgs( | |
| CALL_TYPE_BATCH, | |
| ethers.solidityPacked( | |
| ['bytes4', 'bytes'], | |
| [selector('Error(string)'), coder.encode(['string'], ['CallReceiverMock: reverting'])], | |
| .withArgs(CALL_TYPE_BATCH, encodeErrorString('CallReceiverMock: reverting')) | |
| .to.emit(this.utils, 'return$execBatch') | |
| .withArgs([ | |
| coder.encode( | |
| ['bool', 'bytes'], | |
| [true, this.target.interface.encodeFunctionResult('mockFunction', ['0x1234'])], | |
| ), | |
| ); | |
| coder.encode(['bool', 'bytes'], [false, encodeErrorString('CallReceiverMock: reverting')]), | |
| ]); | |
| await expect(this.utils.$execBatch(data, EXEC_TYPE_TRY)) | |
| .to.emit(this.utils, 'ERC7579TryExecuteFail') | |
| .withArgs(1, encodeErrorString('CallReceiverMock: reverting')) | |
| .to.emit(this.utils, 'return$execBatch') | |
| .withArgs([ | |
| coder.encode( | |
| ['bool', 'bytes'], | |
| [true, this.target.interface.encodeFunctionResult('mockFunction', ['0x1234'])], | |
| ), | |
| coder.encode(['bool', 'bytes'], [false, encodeErrorString('CallReceiverMock: reverting')]), | |
| ]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/account/utils/draft-ERC7579Utils.test.js` around lines 183 - 193, The
test asserts the wrong event argument by using CALL_TYPE_BATCH (which equals
'0x01') instead of the explicit numeric index for the failing item; update the
.withArgs for the ERC7579TryExecuteFail event to use the explicit index 1 (the
second batch item) rather than CALL_TYPE_BATCH so the expected event matches the
actual failing call in this.utils.$execBatch invoked with EXEC_TYPE_TRY; keep
the rest of the .withArgs checks for return$execBatch as-is and ensure you
change only the event index reference (ERC7579TryExecuteFail) to the literal 1.
| await expect(this.utils.$execDelegateCall(data, EXEC_TYPE_TRY)) | ||
| .to.emit(this.utils, 'ERC7579TryExecuteFail') | ||
| .withArgs( | ||
| CALL_TYPE_CALL, | ||
| ethers.solidityPacked( | ||
| ['bytes4', 'bytes'], | ||
| [selector('Error(string)'), coder.encode(['string'], ['CallReceiverMock: reverting'])], | ||
| ), | ||
| ); | ||
| .withArgs(CALL_TYPE_CALL, encodeErrorString('CallReceiverMock: reverting')) | ||
| .to.emit(this.utils, 'return$execDelegateCall') | ||
| .withArgs([coder.encode(['bool', 'bytes'], [false, encodeErrorString('CallReceiverMock: reverting')])]); |
There was a problem hiding this comment.
Same issue: use explicit index 0 instead of CALL_TYPE_CALL.
For delegate call at index 0, the numeric value is correct but the constant is semantically incorrect.
Suggested fix
await expect(this.utils.$execDelegateCall(data, EXEC_TYPE_TRY))
.to.emit(this.utils, 'ERC7579TryExecuteFail')
- .withArgs(CALL_TYPE_CALL, encodeErrorString('CallReceiverMock: reverting'))
+ .withArgs(0, encodeErrorString('CallReceiverMock: reverting'))
.to.emit(this.utils, 'return$execDelegateCall')
.withArgs([coder.encode(['bool', 'bytes'], [false, encodeErrorString('CallReceiverMock: reverting')])]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await expect(this.utils.$execDelegateCall(data, EXEC_TYPE_TRY)) | |
| .to.emit(this.utils, 'ERC7579TryExecuteFail') | |
| .withArgs( | |
| CALL_TYPE_CALL, | |
| ethers.solidityPacked( | |
| ['bytes4', 'bytes'], | |
| [selector('Error(string)'), coder.encode(['string'], ['CallReceiverMock: reverting'])], | |
| ), | |
| ); | |
| .withArgs(CALL_TYPE_CALL, encodeErrorString('CallReceiverMock: reverting')) | |
| .to.emit(this.utils, 'return$execDelegateCall') | |
| .withArgs([coder.encode(['bool', 'bytes'], [false, encodeErrorString('CallReceiverMock: reverting')])]); | |
| await expect(this.utils.$execDelegateCall(data, EXEC_TYPE_TRY)) | |
| .to.emit(this.utils, 'ERC7579TryExecuteFail') | |
| .withArgs(0, encodeErrorString('CallReceiverMock: reverting')) | |
| .to.emit(this.utils, 'return$execDelegateCall') | |
| .withArgs([coder.encode(['bool', 'bytes'], [false, encodeErrorString('CallReceiverMock: reverting')])]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/account/utils/draft-ERC7579Utils.test.js` around lines 248 - 252, The
test uses the semantic constant CALL_TYPE_CALL where the delegate-call result
should assert on the numeric index 0; update the assertion to use the explicit
index 0 instead of CALL_TYPE_CALL in the .withArgs for ERC7579TryExecuteFail
and/or return$execDelegateCall so the expected event payload matches the
delegate-call-at-index-0 shape (keep other values:
encodeErrorString('CallReceiverMock: reverting') and
coder.encode(['bool','bytes'], [false, encodeErrorString('CallReceiverMock:
reverting')]) unchanged). Locate the failing assertion around
this.utils.$execDelegateCall(...) that references CALL_TYPE_CALL and replace it
with 0.
1 participant
Fixes L-38
Alternative to #6419
PR Checklist
npx changeset add)