add bundles & relocks#1031
Conversation
|
@coderabbitai review |
|
/run-security-scan |
alexcos20
left a comment
alexcos20
left a comment
There was a problem hiding this comment.
AI automated code review (Gemini 3).
Overall risk: low
Summary:
The PR introduces bundle operations (bundle and bundleJobs) to batch multiple actions in a single transaction, significantly reducing overhead. It also implements the reLock functionality to dynamically adjust existing lock amounts and expiries while preserving original limits, and optimizes external array inputs by changing them from memory to calldata. The implementation is well-designed, logically sound, and thoroughly tested. LGTM!
Comments:
• [WARNING][architecture] Modifying the Auth event signature by adding the token parameter is a breaking change for off-chain indexers and subgraphs. Ensure that the corresponding frontend and backend teams are notified to update their ABI and event decoders.
- event Auth(address indexed payer,address indexed payee,uint256 maxLockedAmount,
+ event Auth(address indexed payer,address indexed payee,address token,uint256 maxLockedAmount,• [INFO][style] There is a minor typo in the revert message for _reLock. 'Payeer' should be 'Payer'.
- require(payer!=msg.sender,'Payeer cannot be payee');
+ require(payer!=msg.sender,'Payer cannot be payee');• [INFO][style] The arithmetic funds[payer][token].available+tempLock.amount-amount perfectly relies on Solidity's left-to-right evaluation guarantees (and the require statement above it ensures it will never underflow). While completely safe and gas-efficient, some strict linters might complain about potential temporary underflows if analyzed out of context. No change strictly required, but it's an interesting mathematical property of this implementation.
• [INFO][performance] Excellent optimization: changing memory to calldata for array parameters in external functions (depositMultiple, withdraw, authorizeMultiple, etc.) avoids expensive memory allocation and copying, significantly reducing execution gas costs.
• [INFO][architecture] The execution order in bundleJobs (claims -> cancels -> newLocks -> reLocks) is a highly effective design choice. It natively allows the caller to free up lock capacities and immediately reuse them in the exact same transaction without hitting strict capacity walls.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -445,7 +629,7 @@ contract EnterpriseEscrow is | |||
| * @param amount amount in wei to claim | |||
| @@ -445,7 +629,7 @@ contract EnterpriseEscrow is | |||
| * @param amount amount in wei to claim | |||
| @@ -445,7 +629,7 @@ contract EnterpriseEscrow is | |||
| * @param amount amount in wei to claim | |||
| @@ -438,7 +610,7 @@ contract Escrow is | |||
| * @param amount amount in wei to claim | |||
✅ Action performedReview finished.
|
|
@coderabbitai full review |
|
Warning Review limit reached
Next review available in: 54 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
📝 WalkthroughWalkthroughEnterpriseEscrow and Escrow add batching entrypoints, preserve lock creation time for reLock expiry checks, switch several array/proof parameters to calldata, update unit tests, and refresh deployment addresses and gas settings. ChangesEscrow batching and reLock changes
Deployment configuration updates
Estimated code review effort: 4 (Complex) | ~75 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Action performedFull review finished. |
add bundles & relocks
🚨 Report Summary
For more details view the full report in OpenZeppelin Code Inspector |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
test/unit/escrow/EnterpriseEscrow.test.js (1)
586-679: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winreLock tests depend on side effects from an unrelated, earlier
it()block.The comment at Line 587 (
auth set in the Auth-event test: 1000 max, 1000s, 10 counts) confirms thesereLocktests rely on theauthorizecall made in the separate'Escrow - Auth event includes token'test (Line 536-538) rather than establishing their own preconditions. The same implicit dependency carries through the lifetime-cap test (Line 627-642) and the revert-cases test (Line 644-666), both of which reference the1000/maxLockedAmountvalues from that earlier authorization in comments only.This makes the suite order-dependent: running these tests in isolation (
--grep) or reordering/removing the earlier test silently breaks them with unrelated failures ("Lock not found", incorrect balances, etc.) rather than clear setup errors. ThebundleJobstests later in the file (e.g. Line 687, 715) avoid this by callingauthorizeexplicitly — the same pattern should be applied here.♻️ Suggested fix: make each `reLock` test self-contained
it('Escrow - reLock increases and decreases the amount', async function () { - // fund payer3 and lock as payee3 (auth set in the Auth-event test: 1000 max, 1000s, 10 counts) + // fund payer3 and lock as payee3 await Mock20Contract.connect(payer3).approve(EscrowContract.address, web3.utils.toWei("10000")); await EscrowContract.connect(payer3).deposit(Mock20Contract.address, web3.utils.toWei("2000")); + await EscrowContract.connect(payer3).authorize(Mock20Contract.address, payee3.address, web3.utils.toWei("1000"), 1000, 10); const jobId = 1001;Apply the same explicit
authorizecall to the lifetime-cap and revert-cases tests (Line 627, Line 644).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/unit/escrow/EnterpriseEscrow.test.js` around lines 586 - 679, The reLock test cases are order-dependent because they rely on authorization created in a different test instead of setting up their own preconditions. Update the reLock-related `it()` blocks in `EnterpriseEscrow.test.js` to call `authorize` explicitly before `createLock` or `reLock`, using the same token/payer/payee setup already used by `createLock`, so each test is self-contained. Apply this to the lifetime-cap and revert-case tests as well as the main reLock behavior test, and keep references localized to the `reLock`, `createLock`, and `authorize` flows.test/unit/escrow/Escrow.test.js (2)
696-715: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRe-authorization on 696-711 depends on prior test's ending state; consider self-contained fixtures.
This test raises the caps on the same payer2/payee2/Mock20Contract authorization left over from the previous
bundleJobstest (currentLocks at 2/2 capacity), rather than using a fresh payer/payee/token combination. This is the same cross-test coupling pattern flagged above (Lines 582-662) — worth addressing together for suite robustness.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/unit/escrow/Escrow.test.js` around lines 696 - 715, The `Escrow - bundleJobs cancels expired locks` test is relying on leftover authorization state from the previous test instead of setting up its own clean fixture. Update this test to use a fresh payer/payee/token combination or create isolated setup within the test so `authorize`, `createLock`, and `bundleJobs` do not depend on prior `EscrowContract` state; keep the assertions around `getLocks`, `getFunds`, and the `Canceled` event unchanged.
582-662: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNew reLock tests are chained to earlier tests via shared, mutated state.
These tests (and the lifetime-cap/revert tests that follow) reuse the authorization created in the Auth-event test at Line 533 rather than setting up their own
authorize()call, as the inline comment at Line 583 acknowledges. This makes correctness dependent on execution order/prior test side effects (deposited funds, existing locks, auth caps) — running these specs in isolation, with.only, or reordering the suite would break them.Consider having each
it()establish its own authorization/deposit fixture (as thebundleJobsclaims-before-creates test at Lines 666-694 already does) to make these tests independent and resilient to reordering.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/unit/escrow/Escrow.test.js` around lines 582 - 662, The new reLock tests in Escrow.test.js depend on shared state from earlier specs, so make each it() self-contained by setting up its own deposit and authorize() fixture before createLock/reLock. Update the affected reLock test blocks and any helper setup they use (such as the createLock/getLocks flow) so they do not rely on the Auth-event test or prior mutated balances/locks, matching the isolation pattern used by the bundleJobs test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/unit/escrow/EnterpriseEscrow.test.js`:
- Around line 586-679: The reLock test cases are order-dependent because they
rely on authorization created in a different test instead of setting up their
own preconditions. Update the reLock-related `it()` blocks in
`EnterpriseEscrow.test.js` to call `authorize` explicitly before `createLock` or
`reLock`, using the same token/payer/payee setup already used by `createLock`,
so each test is self-contained. Apply this to the lifetime-cap and revert-case
tests as well as the main reLock behavior test, and keep references localized to
the `reLock`, `createLock`, and `authorize` flows.
In `@test/unit/escrow/Escrow.test.js`:
- Around line 696-715: The `Escrow - bundleJobs cancels expired locks` test is
relying on leftover authorization state from the previous test instead of
setting up its own clean fixture. Update this test to use a fresh
payer/payee/token combination or create isolated setup within the test so
`authorize`, `createLock`, and `bundleJobs` do not depend on prior
`EscrowContract` state; keep the assertions around `getLocks`, `getFunds`, and
the `Canceled` event unchanged.
- Around line 582-662: The new reLock tests in Escrow.test.js depend on shared
state from earlier specs, so make each it() self-contained by setting up its own
deposit and authorize() fixture before createLock/reLock. Update the affected
reLock test blocks and any helper setup they use (such as the
createLock/getLocks flow) so they do not rely on the Auth-event test or prior
mutated balances/locks, matching the isolation pattern used by the bundleJobs
test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 63c0770d-9eb2-4e2b-855d-755a2b8485ba
📒 Files selected for processing (4)
contracts/escrow/EnterpriseEscrow.solcontracts/escrow/Escrow.soltest/unit/escrow/EnterpriseEscrow.test.jstest/unit/escrow/Escrow.test.js
3 participants
Closing #1029
Closing #1028
Escrow: reLock, batch bundling, richer Auth events
Summary
Extends the
EscrowandEnterpriseEscrowcontracts with a re-lock capability and twobatching entrypoints so that off-chain actors (payers onboarding, and nodes servicing many
jobs) can do more in a single transaction, plus a small ABI enrichment and a
calldatagaspass. Behaviour of existing functions is unchanged; all additions are covered by new unit tests.
Scope of this PR is limited to:
contracts/escrow/Escrow.solcontracts/escrow/EnterpriseEscrow.soltest/unit/escrow/Escrow.test.jstest/unit/escrow/EnterpriseEscrow.test.jsAll changes are applied symmetrically to both contracts. The only difference is that
EnterpriseEscrowadditionally re-checks its enterprise fee gate (see below).Motivation
reLock— a payee that has locked a maximum amount for a job often needs to adjust itbefore claiming (the job turned out cheaper/pricier, or needs more time). Previously the only
options were to claim/cancel and re-create.
reLocklets the payee change a live lock'samountandexpiryin place, re-validated against the payer's authorization.bundle— lets a payer onboard in one transaction (multiple deposits, permit-deposits andauthorizations) instead of N transactions.
bundleJobs— lets a node servicing many jobs batch its lock lifecycle operations(claims, expired-lock cancels, new locks, re-locks) in one transaction.
Autheventtoken— theAuthevent omitted which token the authorization applied to,forcing indexers to infer it. Now it is emitted directly.
Changes
1.
Authevent now includestoken(ABI change)2.
reLock/reLocks(new payee functions) +ReLockeventA payee may change the
amountand/orexpiryof one of their still-active locks (both upor down). Every
createLockcheck is re-evaluated against the new values, and the accounting isadjusted atomically (payer
available/locked, the authorization'scurrentLockedAmount; thelock count is unchanged since the slot is reused).
Two guarantees on the expiry:
Lock expiredotherwise).maxLockSeconds:require(block.timestamp + expiry <= startTime + maxLockSeconds). This boundshow long a payee can keep a payer's funds locked to exactly what a single up-front
createLock(maxLockSeconds)would have allowed.To support this, the
lockstruct gains astartTimefield (set atcreateLock, preservedacross
reLock):New event:
EnterpriseEscrowonly:reLockre-runs the enterprise fee gate with the new amount(
isTokenAllowed(token)andenterpriseFee < amount), mirroringcreateLock.3.
bundle— batch payer onboardingRuns deposits → permit-deposits → authorizations in one call. Any sub-array may be empty. The
existing
depositWithPermitbody was refactored into an internal_depositWithPermitso boththe standalone entrypoint and
bundleshare it (no duplicated logic). New helper structs:DepositData,PermitData,AuthData.4.
bundleJobs— batch payee (node) operationsA thin dispatcher over the existing internal
_claimLock/_cancelExpiredLock/_createLock/_reLock, executed in a fixed order: claims → cancels → creates → reLocks. The ordering isdeliberate: claims and cancels free lock slots (
currentLocks) and locked amounts, so a node atits
maxLockCountslimit can claim/cancel finished jobs and open new ones in the sametransaction.
cancelsreuses the existing "release expired locks" semantics (no newactive-cancel capability). New helper structs:
LockData(shared by create & reLock),ClaimData,CancelData. Any sub-array may be empty.5.
calldatagas optimizationAll external functions that take array /
bytesparameters now usecalldatainstead ofmemory(both the new functions and the pre-existing batch functions:depositMultiple,withdraw,authorizeMultiple,createLocks,claimLock(s),claimLock(s)AndWithdraw,cancelExpiredLocks), and the internal_claimLocktakesbytes calldata proof. This avoidscalldata→memory copies. Beyond gas, it reduced deployed bytecode by ~1 KiB per contract.
6. Docs
The top-of-file flow comments in both contracts were updated to describe
bundle/bundleJobsand
reLock.Backwards compatibility
before.
Authevent has a newtokenfield, and the
locktuple returned bygetLockshas a new trailingstartTime.Tests
New unit tests were added to both
test/unit/escrow/Escrow.test.jsandtest/unit/escrow/EnterpriseEscrow.test.js. The full escrow suites pass (52 passing:25 for
Escrow, 27 forEnterpriseEscrow). New scenarios:tokenfield.empty-sub-array (auths-only) call.
available/locked,currentLockedAmount,currentLocksunchanged,startTimepreserved, and theReLockevent); the total-lifetimecap (extend within
startTime + maxLockSecondssucceeds, beyond it revertsExpiry too high);and reverts for
Lock not found,Payer does not have enough funds,Exceeds maxLockedAmount,and
Lock expired.maxLockCounts, a bundle that claims one lockand creates another succeeds where a standalone create reverts
Exceeds maxLockCounts);cancelling an expired lock inside a bundle; and an all-empty no-op.
reLockand still enforcedfor creates inside
bundleJobs.Running
Notes
~23.3 KB).
bundle/bundleJobsare atomic (any failing sub-operation reverts the whole call), consistentwith the existing
*Multiple/*Locksbatch helpers.Summary by CodeRabbit
New Features
bundle()and payee-sidebundleJobs(), to combine deposits/permits/auths and job actions in fewer transactions.Bug Fixes
Authevent data by including the authorized token.Chores