fix: support Safe wallets in OVM/splitter deployment flows#226
Conversation
|
Warning Review limit reached
Next review available in: 57 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 Run ID: 📒 Files selected for processing (2)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/splits/safeWalletHelpers.spec.ts (2)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider removing
@ts-nocheckin favor of targeted type casts.
@ts-nocheckdisables all TypeScript checking for the file, which can hide genuine type errors in test setup. Consider usingas unknown as SignerTypecasts on mock objects instead, preserving type safety for the rest of the file.🤖 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/splits/safeWalletHelpers.spec.ts` at line 1, The test file is suppressing all TypeScript checks with `@ts-nocheck`, which hides real issues in the setup. Remove the file-wide suppression and use targeted casts on the mock signer objects instead, such as casting to SignerType via unknown where needed, so the rest of safeWalletHelpers.spec.ts remains type-checked.
50-163: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for timeout and
sendUncheckedTransactionfallback paths.The upstream
executeTransactionSafeAwarehas two untested branches:
Timeout path — when no receipt or Safe event is found within
SAFE_TX_TIMEOUT_MS, it throws'Timed out waiting for the Safe transaction to be executed...'. This is the primary user-facing error when a Safe tx isn't executed yet, so it should be covered.
sendUncheckedTransactionfallback — when the signer doesn't exposesendUncheckedTransaction, the code falls back tosigner.sendTransactionand uses.hash. This path is untested.Reverted direct receipt — when
getTransactionReceiptreturns a receipt withstatus === 0, the code throws'Transaction failed or was reverted'. Not tested.🧪 Suggested additional tests
it('Safe: throws on timeout when no receipt or event is found', async () => { const provider = makeProvider({ getCode: jest.fn().mockResolvedValue('0x6080'), }); const signer = makeSigner(provider, { sendUncheckedTransaction: jest.fn().mockResolvedValue(SAFE_TX_HASH), }); await expect( executeTransactionSafeAware({ signer, ...txArgs }), ).rejects.toThrow('Timed out waiting for the Safe transaction'); }); it('Safe: falls back to sendTransaction when sendUncheckedTransaction is unavailable', async () => { const realReceipt = { status: 1, hash: REAL_TX_HASH }; const provider = makeProvider({ getCode: jest.fn().mockResolvedValue('0x6080'), getTransactionReceipt: jest.fn().mockResolvedValue(realReceipt), }); const signer = makeSigner(provider, { sendTransaction: jest.fn().mockResolvedValue({ hash: REAL_TX_HASH }), }); const result = await executeTransactionSafeAware({ signer, ...txArgs }); expect(result).toBe(realReceipt); expect(signer.sendTransaction).toHaveBeenCalledWith(txArgs); }); it('Safe: throws when direct receipt has status 0 (reverted)', async () => { const provider = makeProvider({ getCode: jest.fn().mockResolvedValue('0x6080'), getTransactionReceipt: jest.fn().mockResolvedValue({ status: 0, hash: REAL_TX_HASH }), }); const signer = makeSigner(provider, { sendUncheckedTransaction: jest.fn().mockResolvedValue(REAL_TX_HASH), }); await expect( executeTransactionSafeAware({ signer, ...txArgs }), ).rejects.toThrow('Transaction failed or was reverted'); });Note: the timeout test may need
jest.useFakeTimers()or a reducedSAFE_TX_TIMEOUT_MSto avoid a long-running test.🤖 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/splits/safeWalletHelpers.spec.ts` around lines 50 - 163, Add tests in executeTransactionSafeAware to cover the missing branches: the Safe timeout path when no receipt/event is found within SAFE_TX_TIMEOUT_MS, the sendUncheckedTransaction fallback to signer.sendTransaction().hash when unchecked sending is unavailable, and the reverted direct-receipt case where getTransactionReceipt returns status 0. Use the existing executeTransactionSafeAware, makeProvider, and makeSigner helpers in safeWalletHelpers.spec.ts, and assert the specific error messages and fallback method calls.
🤖 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/splits/safeWalletHelpers.spec.ts`:
- Line 1: The test file is suppressing all TypeScript checks with `@ts-nocheck`,
which hides real issues in the setup. Remove the file-wide suppression and use
targeted casts on the mock signer objects instead, such as casting to SignerType
via unknown where needed, so the rest of safeWalletHelpers.spec.ts remains
type-checked.
- Around line 50-163: Add tests in executeTransactionSafeAware to cover the
missing branches: the Safe timeout path when no receipt/event is found within
SAFE_TX_TIMEOUT_MS, the sendUncheckedTransaction fallback to
signer.sendTransaction().hash when unchecked sending is unavailable, and the
reverted direct-receipt case where getTransactionReceipt returns status 0. Use
the existing executeTransactionSafeAware, makeProvider, and makeSigner helpers
in safeWalletHelpers.spec.ts, and assert the specific error messages and
fallback method calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6dfe2975-da5b-494f-95ff-525b0499306d
📒 Files selected for processing (3)
src/splits/safeWalletHelpers.tssrc/splits/splitHelpers.tstest/splits/safeWalletHelpers.spec.ts
HananINouman
force-pushed
the
Hanan/safe-tx-receipt-resolution
branch
2 times, most recently
from
6af042b to
9c07cc5
Compare
Safe wallets over WalletConnect return the internal safeTxHash instead of an on-chain transaction hash, so ethers' sendTransaction polling and tx.wait() hang forever and createValidatorManagerAndRewardsSplit / createValidatorManagerAndTotalSplit never resolve, even though the deployment executes on-chain. For contract-wallet signers (detected with the existing isContractAvailable), submit via sendUncheckedTransaction and resolve the OVM address from the factory's CreateObolValidatorManager events for the owner, accepting a candidate only when its transaction is provably ours: its hash equals the wallet-returned hash, or its receipt carries our Safe's ExecutionSuccess(safeTxHash). This makes concurrent or third-party OVM creations for the same owner unmistakable for ours (the factory is permissionless). A backfill query covers executions mined before the wallet call returned. Covers both the rewards-only and split-everything flows; EOA signers keep the unchanged sendTransaction + wait path. Also extract the OVM address from receipts by parsing logs for the factory event instead of positional log indexing, which broke when extra events were interleaved in a transaction. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
HananINouman
force-pushed
the
Hanan/safe-tx-receipt-resolution
branch
from
9c07cc5 to
bc45fed
Compare
|
2 participants
Safe wallets over WalletConnect return the internal safeTxHash instead of an on-chain transaction hash, so ethers' sendTransaction polling and tx.wait() hang forever and createValidatorManagerAndRewardsSplit / createValidatorManagerAndTotalSplit never resolve, even though the deployment executes on-chain.
Summary by CodeRabbit
New Features
Bug Fixes