fix: AllowanceModule start-delay semantics and Safe module diagnostics - #217
Conversation
…isled on-chain createOneTimeAllowanceMetaTransaction passed startAfterInMinutes as the contract's resetBaseMin with resetTimeMin=0, which reverts on-chain for any nonzero value (division by zero in the period alignment) — so a delayed one-time allowance never worked. The parameter is removed. createRecurringAllowanceMetaTransaction documented its last parameter as a relative activation delay, but on-chain resetBaseMin is an absolute epoch-minutes baseline that must be in the past and only aligns period boundaries; a relative value made funds spendable immediately. The parameter is renamed to periodStartBaseInMinutes with truthful docs and a client-side guard against future baselines. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…int8 page cap - SocialRecoveryModule.getGuardians and .nonce reported empty-result errors under the name 'threshold', misdirecting debugging. - getRecoveryRequestEip712Data only accepted a URL string while every sibling method accepts string | Transport | JsonRpcNode. - AllowanceModule.getDelegates with maxNumberOfResults > 255 threw an opaque ABI out-of-bounds error (the module's page size is a uint8); it now fails with a clear RangeError pointing at pagination. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAllowance meta-transactions now use absolute recurring-period baselines, fixed one-time reset fields, and bounded delegate pagination. Social recovery helpers correct operation labels and accept transport or JSON-RPC node instances. ChangesAllowance updates
Social recovery helpers
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/safe/allowanceModule.test.js (1)
21-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the delegate page-size guard.
The new
getDelegatesuint8limit has no regression test. It rejects before RPC access, so it can remain fully offline.Suggested test
describe('AllowanceModule setAllowance encoding', () => { const module = new ak.AllowanceModule(); + test('delegate page size above uint8 range rejects before RPC', async () => { + await expect( + module.getDelegates('http://127.0.0.1:8545', DELEGATE, { + maxNumberOfResults: 256n, + }), + ).rejects.toThrow(RangeError); + }); +🤖 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/safe/allowanceModule.test.js` around lines 21 - 52, Add an offline regression test in the AllowanceModule test suite for getDelegates, verifying that a page size above the uint8 maximum is rejected with RangeError before any RPC access occurs. Use the existing module instance and delegate-related test setup, and preserve coverage for valid page-size behavior if already available.
🤖 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/safe/allowanceModule.test.js`:
- Around line 21-52: Add an offline regression test in the AllowanceModule test
suite for getDelegates, verifying that a page size above the uint8 maximum is
rejected with RangeError before any RPC access occurs. Use the existing module
instance and delegate-related test setup, and preserve coverage for valid
page-size behavior if already available.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e3540c5e-6987-4b9b-945a-cb2614478183
📒 Files selected for processing (3)
src/account/Safe/modules/AllowanceModule.tssrc/account/Safe/modules/SocialRecoveryModule.tstest/safe/allowanceModule.test.js
| startAfterInMinutes: bigint, | ||
| periodStartBaseInMinutes: bigint = 0n, | ||
| ): MetaTransaction { | ||
| const nowInMinutes = BigInt(Math.floor(Date.now() / 60_000)); |
There was a problem hiding this comment.
The contract validates resetBaseMin against block.timestamp when the Safe transaction executes, while this guard uses the caller’s local clock when calldata is created. This can reject a valid queued transaction whose baseline will be in the past by execution time, or accept one that reverts if the local clock is ahead of the chain. Since this is a synchronous calldata builder, could we remove the Date.now() check and document that the baseline must be no later than the chain timestamp at execution? If immediate executability needs validation, that could be a separate async preflight using the latest block timestamp.
| * @param periodStartBaseInMinutes - Optional absolute baseline that period | ||
| * boundaries are aligned to, in minutes since the unix epoch (not a relative | ||
| * delay — the module has no delayed-activation concept and requires this | ||
| * baseline to be in the past). Defaults to 0n, aligning periods to the epoch. |
There was a problem hiding this comment.
0n does not align periods to the epoch. The contract initializes a new allowance to the execution block’s current minute, while an existing allowance keeps its previous lastResetMin. Could we update this documentation accordingly?
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/account/Safe/modules/AllowanceModule.ts (1)
393-400: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValidate the lower bound for
maxNumberOfResults.
uint8accepts only0through255, but negative values bypass this guard and reachbaseGetDelegates. Validate both bounds so invalid page sizes fail consistently with a clearRangeError.Proposed fix
- if (overrides.maxNumberOfResults > 255n) { + if (overrides.maxNumberOfResults < 0n || overrides.maxNumberOfResults > 255n) {🤖 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 `@src/account/Safe/modules/AllowanceModule.ts` around lines 393 - 400, Update the maxNumberOfResults validation in the AllowanceModule delegate-fetch flow to reject values below 0 as well as values above 255, matching the uint8 range. Ensure invalid values throw a clear RangeError before reaching baseGetDelegates, while preserving the existing behavior for valid values and omitted options.
🤖 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.
Outside diff comments:
In `@src/account/Safe/modules/AllowanceModule.ts`:
- Around line 393-400: Update the maxNumberOfResults validation in the
AllowanceModule delegate-fetch flow to reject values below 0 as well as values
above 255, matching the uint8 range. Ensure invalid values throw a clear
RangeError before reaching baseGetDelegates, while preserving the existing
behavior for valid values and omitted options.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 369a6f52-953a-438e-a9e1-51ab620b1cbc
📒 Files selected for processing (2)
src/account/Safe/modules/AllowanceModule.tstest/safe/allowanceModule.test.js
f6962c2 to
df91d8e
Compare
…ll-clock guard - periodStartBaseInMinutes -> inThePastPeriodStartBaseTimeStamp: the parameter now takes a unix timestamp in seconds — the natural input for callers — and is converted to the contract's epoch-minutes resetBaseMin internally (flooring sub-minute precision). The name carries the contract's must-be-in-the-past constraint, and the docs state explicitly that the allowance is active immediately: the value only anchors the period accounting (with a worked example). - the Date.now() guard is removed: it duplicated the contract's own require(resetBaseMin <= currentMin) with a client clock, and blocked legitimately preparing a meta-transaction whose anchor is future at creation but past by execution. A deterministic uint32 range check on the converted minutes replaces it and still catches millisecond-timestamp mistakes. - JSDoc corrections against the deployed contract source: the 0n default anchors the period accounting to execution time for a fresh allowance (not the epoch), and the one-time + baseline revert comes from the explicit resetTimeMin > 0 require, not a division by zero. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
df91d8e to
7b658e5
Compare
The uint8 guard only checked the upper bound; a negative bigint passed through to the ABI encoder and failed with an opaque error instead of the descriptive RangeError. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2 participants
Fixes for the Safe AllowanceModule and SocialRecoveryModule helpers.
createOneTimeAllowanceMetaTransactionpassedstartAfterInMinutesas the contract'sresetBaseMinwithresetTimeMin = 0, which reverts on-chain for any nonzero value (setAllowanceexplicitly requiresresetTimeMin > 0wheneverresetBaseMinis nonzero). The parameter is removed.createRecurringAllowanceMetaTransactionwas documented as a relative activation delay, but on-chainresetBaseMinis an absolute epoch-minutes baseline that must be in the past and only aligns period boundaries — a relative value made funds spendable immediately. Renamed toinThePastPeriodStartBaseTimeStamp, now taking a unix timestamp in seconds (converted to the contract's epoch-minutesresetBaseMininternally, flooring sub-minute precision). The docs state explicitly that the allowance is active immediately — the value only anchors the period accounting, with a worked example. A deterministic uint32 range check on the converted minutes catches millisecond-timestamp mistakes; the contract's ownrequire(resetBaseMin <= currentMin)enforces the past bound at execution time, so a meta-transaction can be prepared before its anchor passes.SocialRecoveryModule.getGuardians/.noncereported empty-result errors under the name "threshold";getRecoveryRequestEip712Dataonly accepted a URL string while every sibling acceptsstring | Transport | JsonRpcNode;AllowanceModule.getDelegateswithmaxNumberOfResults > 255threw an opaque ABI out-of-bounds error (the module's page size is a uint8) and now fails with a clearRangeErrorpointing at pagination.Note: removing/renaming the start-delay parameters is a breaking change to those two helper signatures.
Tests added in
test/safe/allowanceModule.test.js; full offline suite passes.Summary by CodeRabbit
New Features
Bug Fixes
RangeErrorfor out-of-range values.Tests
setAllowance, including baseline conversion edge cases and invalid-input rejection.