fix(starknet): correct SDK relayer conflict handling - #1025
Merged
Conversation
…e creep typescript/.yarn/install-state.gz is a generated Yarn Berry local-state artifact that was committed by mistake; ignore it the same way solidity/.gitignore already does for its own workspace. Also drop the unrelated Python/Rust/build patterns from the root .gitignore, keeping only the .ralph/ entry this pipeline actually needs.
Lock two previously-unexercised branches of handleDepositConflict's status verification: the relayer status endpoint responding with success:false, and the endpoint reporting a numeric status outside the known RelayerDepositStatus range. Both must leave the conflict unverified (statusVerified:false) rather than silently treating an unrecognized outcome as confirmed.
…-reveal-hardening # Conflicts: # .gitignore # typescript/src/lib/starknet/starknet-depositor.ts
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughStarkNet deposit initialization now handles HTTP 409 conflicts with canonical deposit IDs, optional relayer status verification, typed errors, and deterministic endpoint routing. Tests cover the new flow. Generated API references now point to TypeScript sources, and ignore files include Yarn, Python, and build artifacts. ChangesStarkNet relayer conflict flow
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
📝 Generate docstrings
🧪 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 |
lrsaturnino
marked this pull request as ready for review
July 14, 2026 23:10
![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
🤖 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.
Inline comments:
In `@typescript/src/lib/starknet/starknet-depositor.ts`:
- Around line 157-179: Update getDefaultStarkNetRelayerUrl and
getDefaultStarkNetRelayerStatusUrl to resolve
STARKNET_RELAYER_CHAIN_NAMES[chainId] without defaulting to "StarknetTestnet";
throw when the chain ID is unsupported, and reuse a shared helper for the
chain-name resolution so both URL builders behave identically.
- Around line 608-612: Update the response validation in the depositor method
around response.data so it returns undefined unless response.data.success is
exactly true. Preserve returning response.data only when the relayer explicitly
provides the boolean true, rejecting truthy non-boolean values.
- Around line 234-246: Update the enhancedConfig initialization around
relayerUrl and relayerStatusUrl so a default relayerStatusUrl is assigned only
when using the default relayerUrl. If config.relayerUrl is custom and
config.relayerStatusUrl is not explicitly provided, leave status verification
disabled; preserve explicit status URLs and existing defaults when no custom
reveal relayer is configured.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f0222a45-b111-4683-b4e3-05d6e99456ff
📒 Files selected for processing (98)
typescript/.gitignoretypescript/api-reference/README.mdtypescript/api-reference/classes/ArbitrumBitcoinDepositor.mdtypescript/api-reference/classes/ArbitrumExtraDataEncoder.mdtypescript/api-reference/classes/ArbitrumTBTCToken.mdtypescript/api-reference/classes/BaseBitcoinDepositor.mdtypescript/api-reference/classes/BaseTBTCToken.mdtypescript/api-reference/classes/BitcoinClientWithNetworkOverride.mdtypescript/api-reference/classes/BitcoinTxHash.mdtypescript/api-reference/classes/CrossChainDepositor.mdtypescript/api-reference/classes/Deposit.mdtypescript/api-reference/classes/DepositFunding.mdtypescript/api-reference/classes/DepositRefund.mdtypescript/api-reference/classes/DepositScript.mdtypescript/api-reference/classes/DepositsService.mdtypescript/api-reference/classes/ElectrumClient.mdtypescript/api-reference/classes/EthereumAddress.mdtypescript/api-reference/classes/EthereumBridge.mdtypescript/api-reference/classes/EthereumDepositorProxy.mdtypescript/api-reference/classes/EthereumExtraDataEncoder.mdtypescript/api-reference/classes/EthereumL1BitcoinDepositor.mdtypescript/api-reference/classes/EthereumL1BitcoinRedeemer.mdtypescript/api-reference/classes/EthereumTBTCToken.mdtypescript/api-reference/classes/EthereumTBTCVault.mdtypescript/api-reference/classes/EthereumWalletRegistry.mdtypescript/api-reference/classes/Hex.mdtypescript/api-reference/classes/MaintenanceService.mdtypescript/api-reference/classes/OptimisticMinting.mdtypescript/api-reference/classes/RedemptionsService.mdtypescript/api-reference/classes/RelayerDepositConflictError.mdtypescript/api-reference/classes/SeiAddress.mdtypescript/api-reference/classes/SeiBitcoinDepositor.mdtypescript/api-reference/classes/SeiExtraDataEncoder.mdtypescript/api-reference/classes/SeiTBTCToken.mdtypescript/api-reference/classes/SolanaAddress.mdtypescript/api-reference/classes/SolanaExtraDataEncoder.mdtypescript/api-reference/classes/Spv.mdtypescript/api-reference/classes/StarkNetAddress.mdtypescript/api-reference/classes/StarkNetBitcoinDepositor.mdtypescript/api-reference/classes/StarkNetExtraDataEncoder.mdtypescript/api-reference/classes/StarkNetTBTCToken.mdtypescript/api-reference/classes/TBTC.mdtypescript/api-reference/classes/TBTCCore.mdtypescript/api-reference/classes/WalletTx.mdtypescript/api-reference/enums/ApiUrl.mdtypescript/api-reference/enums/BitcoinNetwork-1.mdtypescript/api-reference/enums/Chains.Arbitrum.mdtypescript/api-reference/enums/Chains.Base.mdtypescript/api-reference/enums/Chains.Ethereum.mdtypescript/api-reference/enums/Chains.Sei.mdtypescript/api-reference/enums/Chains.Solana.mdtypescript/api-reference/enums/Chains.StarkNet.mdtypescript/api-reference/enums/Chains.Sui.mdtypescript/api-reference/enums/DepositState.mdtypescript/api-reference/enums/RelayerDepositStatus.mdtypescript/api-reference/enums/WalletState-1.mdtypescript/api-reference/enums/endpointUrl.mdtypescript/api-reference/interfaces/BitcoinClient.mdtypescript/api-reference/interfaces/BitcoinDepositor.mdtypescript/api-reference/interfaces/BitcoinHeader.mdtypescript/api-reference/interfaces/BitcoinRawTx.mdtypescript/api-reference/interfaces/BitcoinRawTxVectors.mdtypescript/api-reference/interfaces/BitcoinSpvProof.mdtypescript/api-reference/interfaces/BitcoinTx.mdtypescript/api-reference/interfaces/BitcoinTxMerkleBranch.mdtypescript/api-reference/interfaces/BitcoinTxOutpoint.mdtypescript/api-reference/interfaces/BitcoinTxOutput.mdtypescript/api-reference/interfaces/Bridge.mdtypescript/api-reference/interfaces/ChainEvent.mdtypescript/api-reference/interfaces/ChainIdentifier.mdtypescript/api-reference/interfaces/CrossChainContractsLoader.mdtypescript/api-reference/interfaces/DepositReceipt.mdtypescript/api-reference/interfaces/DepositRequest.mdtypescript/api-reference/interfaces/DepositorProxy.mdtypescript/api-reference/interfaces/DestinationChainTBTCToken.mdtypescript/api-reference/interfaces/ElectrumCredentials.mdtypescript/api-reference/interfaces/EthereumContractConfig.mdtypescript/api-reference/interfaces/ExtraDataEncoder.mdtypescript/api-reference/interfaces/GetChainEvents.Function.mdtypescript/api-reference/interfaces/GetChainEvents.Options.mdtypescript/api-reference/interfaces/L1BitcoinRedeemer.mdtypescript/api-reference/interfaces/L2BitcoinRedeemer.mdtypescript/api-reference/interfaces/RedeemerProxy.mdtypescript/api-reference/interfaces/RedemptionRequest.mdtypescript/api-reference/interfaces/RedemptionWallet.mdtypescript/api-reference/interfaces/SeiBitcoinDepositorConfig.mdtypescript/api-reference/interfaces/SerializableWallet.mdtypescript/api-reference/interfaces/StarkNetBitcoinDepositorConfig.mdtypescript/api-reference/interfaces/StarkNetTBTCTokenConfig.mdtypescript/api-reference/interfaces/TBTCToken.mdtypescript/api-reference/interfaces/TBTCVault.mdtypescript/api-reference/interfaces/ValidRedemptionWallet.mdtypescript/api-reference/interfaces/Wallet.mdtypescript/api-reference/interfaces/WalletRegistry.mdtypescript/api-reference/modules/BitcoinNetwork.mdtypescript/api-reference/modules/WalletState.mdtypescript/src/lib/starknet/starknet-depositor.tstypescript/test/lib/starknet/starknet-depositor-implementation.test.ts
piotr-roslaniec
left a comment
piotr-roslaniec
left a comment
Contributor
There was a problem hiding this comment.
This review is framed as questions for the author. The core change, where a 409 no longer fabricates a success Hex and instead always throws a typed RelayerDepositConflictError, reads as correct and well-motivated. A few questions below, most inline.
Propagation test coverage
The stated goal is that a 409 surfaces a typed throw to the SDK consumer (CrossChainDepositor.revealDeposit) instead of a fabricated Hex. The 409 tests in typescript/test/lib/starknet/starknet-depositor-implementation.test.ts assert one level lower, at StarkNetBitcoinDepositor.initializeDeposit. Propagation currently holds only because revealDeposit (typescript/src/services/deposits/cross-chain.ts:89-131) has no try/catch, but nothing pins that. Would a test that wraps the depositor in CrossChainDepositor and asserts RelayerDepositConflictError propagates out of revealDeposit be worth adding, so a future change inside revealDeposit cannot silently swallow the error?
Dead 409 branch in isRetryableError
typescript/src/lib/starknet/starknet-depositor.ts:676 still special-cases error.response?.status === 409, but a 409 is now short-circuited into handleDepositConflict at :478 before this formatter runs. Is that branch still reachable, or is it dead now and safe to remove? (It falls outside the diff hunks, so noting it here rather than inline.)
Close the tbtcv2-01 gap where an unrecognized chain ID combined with only a custom relayerUrl (no relayerStatusUrl) constructed successfully instead of failing. A custom/unsupported StarkNet network - one with no default relayer route - is now supported only when the caller supplies BOTH the reveal and status endpoints explicitly; otherwise construction throws synchronously before any Axios request, preventing a reveal from being misrouted after Bitcoin funding. Recognized chain IDs keep their documented defaulting, including the supported recognized-chain custom-reveal-only case where status verification is simply disabled. Replace the reveal-only unknown-chain not-throw test with a synchronous rejection assertion, and add a table-driven test pinning the exact paired reveal and status routes (matching chain segments) for mainnet, Sepolia, and the legacy Goerli mapping.
Chain-name recognition indexed STARKNET_RELAYER_CHAIN_NAMES directly, so chain IDs matching Object.prototype members (toString, __proto__, constructor, valueOf) resolved to truthy inherited members and appeared to have a default relayer route. Such an ID bypassed the unknown-chain both-endpoint rule and got interpolated into a malformed relayer URL instead of failing loudly after Bitcoin funding. Add a single authoritative, own-property-safe hasDefaultStarkNetRelayerRoute (hasOwnProperty.call) and reuse it in both resolveStarkNetRelayerChainName and the constructor's both-endpoint rule. Add regression tests for toString/__proto__/constructor with no overrides and each single-endpoint config (all throw) plus the both-explicit-endpoints success case.
The Starknet SDK hardening changes to starknet-depositor.ts (RelayerDepositStatus JSDoc clarifying the three on-chain states vs. the relayer's internal lifecycle enum, the isRetryableError 409-conflict note, and the resulting symbol relocations) were not accompanied by regenerating the committed typedoc output. This resync makes the typescript-docs CI job (yarn build && yarn docs && git diff --exit-code) pass again. Generated via yarn docs; no hand-edits.
…ardening # Conflicts: # typescript/api-reference/README.md # typescript/api-reference/classes/EthereumExtraDataEncoder.md # typescript/api-reference/classes/EthereumL1BitcoinDepositor.md # typescript/api-reference/classes/SeiAddress.md # typescript/api-reference/classes/SeiBitcoinDepositor.md # typescript/api-reference/classes/SeiExtraDataEncoder.md # typescript/api-reference/classes/SeiTBTCToken.md # typescript/api-reference/enums/Chains.Sei.md # typescript/api-reference/interfaces/SeiBitcoinDepositorConfig.md
- Add SN_TESTNET chain ID to the relayer chain-name allow-list, fixing a construction-time regression for a chain ID already declared supported in index.ts's TBTC_CONTRACT_ADDRESSES - Wire relayerStatusUrl through loadStarkNetCrossChainInterfaces (new optional parameter + STARKNET_RELAYER_STATUS_URL env var), so 409 conflict-status verification is reachable through the public loader even when STARKNET_RELAYER_URL is set - Cross-check the relayer-reported deposit ID against the SDK's own derivation in handleDepositConflict, warning (not gating) on mismatch - Rename RelayerDepositStatus/RelayerDepositConflictError to StarkNetRelayerDepositStatus/StarkNetRelayerDepositConflictError to match this module's StarkNet-prefixed export convention - Strip trailing slashes before building the deposit-status URL - Fix a setTimeout stub leak in a test that could cascade failures into unrelated tests - Fix expectConflictError test helper swallowing its own expect.fail - Add tests for the relayerStatusUrl-only defaulting combo and the default status URL's local-origin branch - Fix 4 misleading test titles, a stale numeric claim in a test comment, and stale @see references; add a buildDepositKey cross-reference and document self-referential hash test vectors as self-consistency guards - Disclose the construction-time breaking change, the derivation correction, and correct the local-relayer wording in the PR description - Regenerate API docs for the renamed error/enum types
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
typescript/test/lib/starknet/starknet-depositor-payload.test.ts (1)
248-260: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake all
console.logstub cleanup failure-safe.Each test restores a process-global Sinon stub only after assertions. A failure leaves the stub installed and can corrupt later tests.
typescript/test/lib/starknet/starknet-depositor-payload.test.ts#L248-L260: restoreconsoleLogStubinfinally.typescript/test/lib/starknet/starknet-depositor-payload.test.ts#L263-L290: restoreconsoleLogStubinfinally.typescript/test/lib/starknet/starknet-depositor-payload.test.ts#L293-L319: restoreconsoleLogStubinfinally.🤖 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 `@typescript/test/lib/starknet/starknet-depositor-payload.test.ts` around lines 248 - 260, Wrap the assertion blocks using consoleLogStub in try/finally so consoleLogStub.restore() always executes, including when assertions fail. Apply this to all three affected sites in typescript/test/lib/starknet/starknet-depositor-payload.test.ts: lines 248-260, 263-290, and 293-319; each site requires the same failure-safe cleanup change.
🤖 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.
Inline comments:
In `@typescript/api-reference/enums/StarkNetRelayerDepositStatus.md`:
- Around line 5-13: Update the StarkNetRelayerDepositStatus documentation to
explicitly state that the public QUEUED = 0 value maps to the L1 depositor
contract’s Unknown state, clarifying it is not a relayer lifecycle queue state.
Preserve the existing three-state scope and fail-closed validation description.
In `@typescript/test/lib/starknet/starknet-depositor-implementation.test.ts`:
- Around line 1271-1320: Update the test around
StarkNetDepositor.initializeDeposit to save the original global.window property
state before assigning the localhost mock, then restore or delete it in a
finally block regardless of test outcome. Keep the existing assertions and
conflict-error handling unchanged.
In `@typescript/test/lib/starknet/starknet-depositor-payload.test.ts`:
- Around line 206-214: Replace the self-generated expected values in the
affected Starknet depositor payload tests with an independently verified
canonical deposit-ID vector, including the serialized transaction, output index,
and deposit ID from the on-chain depositor or a trusted independent
implementation. Update the tests around deriveCanonicalDepositId while
preserving coverage for the remaining mock vectors.
---
Outside diff comments:
In `@typescript/test/lib/starknet/starknet-depositor-payload.test.ts`:
- Around line 248-260: Wrap the assertion blocks using consoleLogStub in
try/finally so consoleLogStub.restore() always executes, including when
assertions fail. Apply this to all three affected sites in
typescript/test/lib/starknet/starknet-depositor-payload.test.ts: lines 248-260,
263-290, and 293-319; each site requires the same failure-safe cleanup change.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 436de011-90bd-42f8-8535-8003a937933d
📒 Files selected for processing (15)
typescript/api-reference/README.mdtypescript/api-reference/classes/EthereumExtraDataEncoder.mdtypescript/api-reference/classes/EthereumL1BitcoinDepositor.mdtypescript/api-reference/classes/StarkNetBitcoinDepositor.mdtypescript/api-reference/classes/StarkNetRelayerDepositConflictError.mdtypescript/api-reference/enums/StarkNetRelayerDepositStatus.mdtypescript/api-reference/interfaces/BitcoinDepositor.mdtypescript/api-reference/interfaces/ChainIdentifier.mdtypescript/api-reference/interfaces/DestinationChainTBTCToken.mdtypescript/api-reference/interfaces/ExtraDataEncoder.mdtypescript/api-reference/interfaces/StarkNetBitcoinDepositorConfig.mdtypescript/src/lib/starknet/index.tstypescript/src/lib/starknet/starknet-depositor.tstypescript/test/lib/starknet/starknet-depositor-implementation.test.tstypescript/test/lib/starknet/starknet-depositor-payload.test.ts
💤 Files with no reviewable changes (4)
- typescript/api-reference/interfaces/DestinationChainTBTCToken.md
- typescript/api-reference/interfaces/ExtraDataEncoder.md
- typescript/api-reference/interfaces/ChainIdentifier.md
- typescript/api-reference/interfaces/BitcoinDepositor.md
🚧 Files skipped from review as they are similar to previous changes (3)
- typescript/api-reference/classes/EthereumExtraDataEncoder.md
- typescript/api-reference/classes/EthereumL1BitcoinDepositor.md
- typescript/src/lib/starknet/starknet-depositor.ts
Comment on lines
+5
to
+13
| These are the three on-chain states the Starknet deposit-status endpoint can | ||
| return. The endpoint derives its value from the L1 depositor contract's | ||
| `deposits` mapping (`Unknown`/`Initialized`/`Finalized`), which has exactly | ||
| these three states. This enum intentionally does NOT mirror every value of | ||
| the relayer's internal cross-chain lifecycle enum (which additionally has | ||
| `AWAITING_WORMHOLE_VAA` and `BRIDGED`): those internal lifecycle values are | ||
| never surfaced by the status endpoint, so trusting them here would widen the | ||
| accepted input beyond what the endpoint can actually report and weaken the | ||
| fail-closed validation in [StarkNetBitcoinDepositor.handleDepositConflict](../classes/StarkNetBitcoinDepositor.md#handledepositconflict). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document the zero-state mapping.
The text identifies the L1 state as Unknown, but the public enum exposes QUEUED = 0. State that QUEUED maps to the L1 Unknown state, or align the terminology. This prevents consumers from treating it as a relayer lifecycle queue state.
🤖 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 `@typescript/api-reference/enums/StarkNetRelayerDepositStatus.md` around lines
5 - 13, Update the StarkNetRelayerDepositStatus documentation to explicitly
state that the public QUEUED = 0 value maps to the L1 depositor contract’s
Unknown state, clarifying it is not a relayer lifecycle queue state. Preserve
the existing three-state scope and fail-closed validation description.
Comment on lines
+1271
to
+1320
| ;(global as any).window = { location: { hostname: "localhost" } } | ||
|
|
||
| const mockProvider = createMockProvider() | ||
| const depositor = new StarkNetDepositor( | ||
| { chainId: "0x534e5f4d41494e" }, | ||
| "StarkNet", | ||
| mockProvider | ||
| ) | ||
| depositor.setDepositOwner(StarkNetAddress.from("0x123")) | ||
|
|
||
| const mockDepositTx = createMockDepositTx() | ||
| const mockReceipt = createMockDeposit() | ||
|
|
||
| const conflictError: any = new Error( | ||
| "Request failed with status code 409" | ||
| ) | ||
| conflictError.isAxiosError = true | ||
| conflictError.response = { | ||
| status: 409, | ||
| data: { | ||
| success: false, | ||
| error: "Deposit already exists", | ||
| depositId: "123456789", | ||
| }, | ||
| } | ||
| axios.post = sinon.stub().rejects(conflictError) | ||
|
|
||
| let capturedStatusUrl = "" | ||
| axios.get = sinon.stub().callsFake((url: string) => { | ||
| capturedStatusUrl = url | ||
| return Promise.resolve({ | ||
| data: { | ||
| success: true, | ||
| depositId: "123456789", | ||
| status: StarkNetRelayerDepositStatus.QUEUED, | ||
| }, | ||
| }) | ||
| }) | ||
|
|
||
| try { | ||
| await depositor.initializeDeposit(mockDepositTx, 0, mockReceipt) | ||
| expect.fail("Should have thrown StarkNetRelayerDepositConflictError") | ||
| } catch (err) { | ||
| expect(err).to.be.instanceOf(StarkNetRelayerDepositConflictError) | ||
| } | ||
|
|
||
| expect(capturedStatusUrl).to.equal( | ||
| "http://localhost:3001/api/StarknetMainnet/deposit/123456789" | ||
| ) | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Restore global.window after this test.
This test replaces the process-global window object and does not restore it. Later tests can incorrectly select localhost relayer routing. Save the prior property state and restore it in finally.
🤖 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 `@typescript/test/lib/starknet/starknet-depositor-implementation.test.ts`
around lines 1271 - 1320, Update the test around
StarkNetDepositor.initializeDeposit to save the original global.window property
state before assigning the localhost mock, then restore or delete it in a
finally block regardless of test outcome. Keep the existing assertions and
conflict-error handling unchanged.
Comment on lines
+206
to
+214
| // The three exact-decimal expected values pinned below (in this test and | ||
| // the two that follow) were generated by running deriveCanonicalDepositId | ||
| // itself against the mock vectors, not sourced from an independent, | ||
| // relayer-confirmed ground truth. They are self-consistency regression | ||
| // guards - they catch an accidental future change to the derivation - not | ||
| // a proof that the derivation matches the real relayer's output. The | ||
| // formula itself (double-SHA256, unreversed, packed with a uint32 output | ||
| // index) is independently cross-checked against this SDK's on-chain | ||
| // deposit-key convention in deriveCanonicalDepositId's JSDoc. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use an independent canonical deposit-ID vector.
The expected values were generated by deriveCanonicalDepositId. These tests can pass if the production derivation is wrong in the same way. Add a (serialized transaction, output index, deposit ID) vector from the on-chain depositor or another known-good implementation.
🤖 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 `@typescript/test/lib/starknet/starknet-depositor-payload.test.ts` around lines
206 - 214, Replace the self-generated expected values in the affected Starknet
depositor payload tests with an independently verified canonical deposit-ID
vector, including the serialized transaction, output index, and deposit ID from
the on-chain depositor or a trusted independent implementation. Update the tests
around deriveCanonicalDepositId while preserving coverage for the remaining mock
vectors.
…ardening # Conflicts: # typescript/api-reference/README.md # typescript/api-reference/classes/EthereumBridge.md # typescript/api-reference/classes/EthereumWalletRegistry.md # typescript/api-reference/classes/RedemptionsService.md # typescript/api-reference/enums/endpointUrl.md # typescript/api-reference/interfaces/WalletRegistry.md
The merge introduced generated-doc conflicts resolved in favor of main's committed content; this regenerates them against the actual merged source so file-path links (and StarkNet rename fallout) are consistent.
Apply 26 confirmed review findings from multi-agent review of PR #1025 (Starknet strict-fee-check rebased): P1 fixes: - Root .gitignore: anchor target/ and build/ patterns so they do not defeat subsystem .gitignore negation rules - deposit-id derivation: add EthereumBridge.buildDepositKey cross-check test to catch future drift from the on-chain Deposit.sol formula P2 fixes: - Constructor chain-ID guard: decouple relayerUrl from relayerStatusUrl (unrecognized chain only requires relayerUrl) - handleDepositConflict: surface locallyDerivedDepositId when relayer's ID is non-canonical; downgrade statusVerified on ID mismatch - Localhost routing: exclude Chains.StarkNet.Mainnet from localhost- default path so mainnet always uses the production relayer - loadStarkNetCrossChainInterfaces: emit one-time warning when STARKNET_RELAYER_URL disables status verification - StarkNetRelayerDepositStatus enum: document QUEUED=0 as on-chain Unknown; soften QUEUED user-facing message - initializeDeposit JSDoc: state it always resolves with full receipt - Constructor JSDoc: add @throws clause and @PARAM config note - StarkNetRelayerDepositConflictError: document statusVerified semantics P3 fixes: - TBTC_CONTRACT_ADDRESSES: prototype-safe lookup mirroring depositor guard - 409 JSDoc: reword to conditional verification - getDefaultStarkNetRelayerUrl comment: note defense-in-depth role - resolveStarkNetRelayerChainName JSDoc: enumerate all 4 chains - Tests: include SN_TESTNET in recognized-chain arrays - Tests: align assertions with new F3 mismatch-downgrade behavior - Tests: replace non-hex transactionHash fakes with valid 64-char hex - Tests: fix prototype-collision + chain-ID-guard test expectations to reflect decoupled relayerUrl/relayerStatusUrl rules
Two git checkout HEAD -- reverts used earlier to recover from broken
multi-line edits also discarded already-applied P3 findings. Re-apply
the ones that did not survive:
- Add SN_TESTNET (0x534e5f544553544e4554) to the recognizedChainRoutes
and recognizedChainIds test arrays in
starknet-depositor-implementation.test.ts, matching all 4 entries in
STARKNET_RELAYER_CHAIN_NAMES. Tests previously claimed to cover
every recognized chain ID while omitting one - the exact defect the
review flagged.
- Fix the quoted-title mismatch in the same file: a comment quoted a
test title ('custom reveal URL without an explicit status URL'
regression test) that does not match any actual it() title; now
names the real title ('should not query a default status endpoint
when only a custom reveal URL is configured').
- Replace the remaining position-based comment ('the two that follow')
in starknet-depositor-payload.test.ts with the actual test titles it
refers to, and reword the still-present 'must derive exactly the ID
the relayer derives' claim in the same file to match the corrected
self-consistency-guard framing used elsewhere in the same comment
block.
…ange The F3 fix (query status using the locally-derived deposit ID when the relayer's reported ID is missing/non-canonical, and downgrade statusVerified on a mismatch) changed handleDepositConflict's actual behavior without updating two JSDoc blocks that still described the pre-F3 rules: - initializeDeposit's JSDoc said verification only happens 'when relayerStatusUrl is configured and the relayer reported a canonical deposit ID' - false since F3 also queries using the SDK's own derived ID as a fallback. - handleDepositConflict's @PARAM locallyDerivedDepositId doc said it is 'used only to warn on a mismatch ... never to replace it or gate the status query' - the opposite of what F3 does (it does gate/seed the query, and drives the mismatch-downgrade, not just a warning). Reword both to state the actual fallback-to-local-ID and mismatch-downgrade behavior. No behavior change; regenerated api-reference for the initializeDeposit doc text.
F6 was only half-applied: loadStarkNetCrossChainInterfaces gained a
4th relayerStatusUrl parameter, but the SDK's primary entry point
(TBTC.initializeCrossChain) still called it with 3 args, so the
parameter was reachable only by calling the loader directly - the
exact 'unreachable from the primary entry point' defect the review
named. The one-time constructor warning added earlier tells a caller
verification got disabled, but doesn't give them a way to fix it
through initializeCrossChain itself.
- Add an optional third 'options: { relayerStatusUrl?: string }'
parameter to TBTC.initializeCrossChain and pass it through in the
StarkNet case. Purely additive - every existing 2-arg call site is
unaffected, and the parameter is ignored for all other L2 chains.
- Add a regression test asserting a custom relayerStatusUrl passed via
initializeCrossChain reaches the constructed depositor's actual
status-endpoint GET target on a 409 conflict.
- Fix an orphaned 'so a / conflict never / resolves to...' line wrap
in initializeDeposit's JSDoc left over from an earlier edit.
- Regenerate api-reference (TBTC.md, StarkNetBitcoinDepositor.md) for
the changed public signatures/doc text.
piotr-roslaniec
added a commit
that referenced
this pull request
Aug 31, 2026
Regenerates the TypeScript api-reference docs from the merged source to resolve conflicts between this branch's NTT migration doc regen and dev's StarkNet relayer conflict-handling doc regen (#1025). No manual content was hand-merged; the api-reference tree is the typedoc output for the merged src/.
piotr-roslaniec
added a commit
that referenced
this pull request
Aug 31, 2026
Resolves conflicts against dev's fixture-snapshot fix (#1054) and starknet .gitignore additions (#1025): - .gitignore: keep both agent-docs/ and dev's new entries - AbstractL1BTCDepositor.test.ts, L1BTCDepositorWormholeV2Base.test.ts: adopt dev's loadFixture snapshot-verification fix while keeping this PR's reimbursementPool/initializer additions
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
When the relayer responded with HTTP 409 to an already-in-flight deposit, the SDK returned the relayer's deposit ID as a Hex, consumed as a reveal tx hash. The conflict lookup also trusted whatever deposit ID and status the relayer returned without validating it was genuine. Separately, a dead branch left local-relayer development routing permanently disabled.
Solution
Every 409 now routes through a single handler that always throws a typed
StarkNetRelayerDepositConflictError(renamed fromRelayerDepositConflictError/RelayerDepositStatusto match this module's StarkNet-prefixed export convention); there is no longer a 409 path that returns a partial object as a successful receipt. The handler validates the deposit ID's shape against a canonical-integer/uint256-range pattern, independently re-derives the deposit ID from the funding transaction and warns on (and downgrades) a mismatch against the relayer's reported ID, and only marks a status verified when the relayer's response echoes the same ID and reports one of the three recognized on-chain states. Deposit-ID derivation itself was also corrected as part of this fix: the previous code mislabeled a double-keccak hash as "double SHA-256" and packed the output index asuint256; it now uses a real Bitcoin SHA-256d digest and packs the index asuint32, matching this SDK's on-chain deposit-key formula (EthereumBridge.buildDepositKey) - this changes the exact decimal value logged for every deposit ID. StarkNet chain-ID recognition is now an explicit allow-list (rejecting prototype-pollution-style keys such as"toString"/"__proto__"and unrecognized/mistyped chain IDs) instead of a silentmap[key] || "StarknetTestnet"fallback. Local-relayer development routing was re-enabled by removing the dead&& falseguard that had permanently disabled it (this activates the local-dev routing path; it does not remove any code path). Generated API docs were updated for the new error type.Breaking Changes
StarkNetBitcoinDepositor(directly, or vialoadStarkNetCrossChainInterfaces) with an unrecognized chain ID now throws synchronously at construction time instead of silently defaulting to the testnet relayer route. Only chain IDs in the SDK's known StarkNet chain-ID map (mainnet, Sepolia, legacy Goerli) default correctly; a caller passing any other chain ID (including non-hex identifiers like the string"SN_MAIN", which never worked as intended) must now supply bothrelayerUrlandrelayerStatusUrlexplicitly.relayerStatusUrlfield onStarkNetBitcoinDepositorConfig, also newly accepted as an optional parameter onloadStarkNetCrossChainInterfaces(falls back toSTARKNET_RELAYER_STATUS_URL).RelayerDepositStatusandRelayerDepositConflictErrorare renamed toStarkNetRelayerDepositStatusandStarkNetRelayerDepositConflictErrorto match this module's naming convention (both are new in this PR, so this is a rename of unreleased API, not a break of prior behavior).error instanceof StarkNetRelayerDepositConflictErrorand inspectdepositId,status, andstatusVerified.trueis accepted; truthy non-boolean values such as 'true', 1, {}, or [] now throw instead of resolving to a receipt. The deployed relayer emits a JSON boolean.Tests
New unit tests cover every terminal relayer status arriving via 409, malformed and mismatched deposit IDs, restored local-relayer routing, the
relayerStatusUrl-only defaulting combination, and the default status endpoint's local-vs-production origin detection.yarn build,yarn test, andyarn formatpass fromtypescript/(755+ tests); docs were regenerated and confirmed to match the committed output.Summary by CodeRabbit
New Features
Documentation
Tests
Chores