SDK: harden network and response binding#960
Open
mswilkison wants to merge 5 commits into
Open
Conversation
mswilkison
force-pushed
the
codex/sdk-client-hardening
branch
from
a915f7b to
5b4f837
Compare
📝 WalkthroughWalkthroughThis PR enhances cross-chain deposit flows with mandatory provider validation, stricter address bounds checking, and deposit owner extraction from transaction data. Key changes include Electrum transaction hash verification, Solana genesis hash validation, StarkNet field prime address validation, StarkNet chain ID enforcement, and consistent deposit owner derivation across Solana, Sui, and StarkNet chains. ChangesCross-Chain Deposit Validation & Owner Extraction
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
typescript/src/lib/starknet/starknet-depositor.ts (1)
111-121:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winAccept canonical StarkNet chain ID aliases in relayer URL mapping.
Line [118] looks up
chainNameMapwith rawconfig.chainId, but the current map shape rejects valid alias inputs (SN_MAIN), which is already causing runtime construction failures in CI.💡 Proposed fix
- const chainNameMap: Record<string, string> = { + const chainNameMap: Record<string, string> = { + SN_GOERLI: "StarknetTestnet", + SN_SEPOLIA: "StarknetTestnet", + SN_MAIN: "StarknetMainnet", "0x534e5f474f45524c49": "StarknetTestnet", // SN_GOERLI "0x534e5f5345504f4c4941": "StarknetTestnet", // SN_SEPOLIA - mapped to StarknetTestnet in relayer "0x534e5f4d41494e": "StarknetMainnet", // SN_MAIN "0x534e5f544553544e4554": "StarknetTestnet", // Test chain ID } - - const chainName = chainNameMap[config.chainId] + const normalizedChainId = config.chainId.startsWith("0x") + ? config.chainId.toLowerCase() + : config.chainId.toUpperCase() + const chainName = chainNameMap[normalizedChainId] if (!chainName) { throw new Error(`Unsupported StarkNet chain ID: ${config.chainId}`) }🤖 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/src/lib/starknet/starknet-depositor.ts` around lines 111 - 121, The chain lookup in chainNameMap using config.chainId fails for canonical alias inputs like "SN_MAIN"; update the logic so aliases are accepted by either adding alias keys ("SN_GOERLI", "SN_SEPOLIA", "SN_MAIN") to chainNameMap mapping to the same values as their hex counterparts, or normalize config.chainId before lookup (e.g., translate known aliases to their hex form) so the existing lookup for chainName succeeds; adjust the code that computes chainName (variable chainName and map chainNameMap) accordingly to ensure both hex IDs and canonical alias strings resolve to "StarknetMainnet" or "StarknetTestnet" as intended.
🧹 Nitpick comments (1)
typescript/test/lib/starknet/starknet-depositor-payload.test.ts (1)
245-280: ⚡ Quick winRestore
console.login afinallyblock to avoid cross-test leakage.If this test fails before Line [279], the stub can persist and affect later tests.
🧪 Proposed hardening
it("should not log reveal request payloads", async () => { const consoleLogStub = sinon.stub(console, "log") - - axiosStub.resolves({ - data: { - success: true, - receipt: { - transactionHash: - "0x366220f9853aa8ad83376bcb3fd9377da7b55f03fc3a3aa4aed7b57f7cc60745", - blockNumber: 8486402, - }, - }, - }) - - const depositTx: BitcoinRawTxVectors = { - version: Hex.from("02000000"), - inputs: Hex.from("01" + "a".repeat(64)), - outputs: Hex.from("02" + "e".repeat(64)), - locktime: Hex.from("00000000"), - } - - const deposit: DepositReceipt = { - depositor: EthereumAddress.from("0x" + "0".repeat(40)), - walletPublicKeyHash: Hex.from("ef5a2946f294f1742a779c9ac034bc3fa5d417b8"), - refundPublicKeyHash: Hex.from("b4f19a044feea3aa4a7d3f494433a11d0f1c400e"), - blindingFactor: Hex.from("b3460f26eda61ad1"), - refundLocktime: Hex.from("a1faa569"), - extraData: Hex.from(testAddress), - } - - await depositor.initializeDeposit(depositTx, 0, deposit) - - expect(consoleLogStub.called).to.be.false - - consoleLogStub.restore() + try { + axiosStub.resolves({ + data: { + success: true, + receipt: { + transactionHash: + "0x366220f9853aa8ad83376bcb3fd9377da7b55f03fc3a3aa4aed7b57f7cc60745", + blockNumber: 8486402, + }, + }, + }) + + const depositTx: BitcoinRawTxVectors = { + version: Hex.from("02000000"), + inputs: Hex.from("01" + "a".repeat(64)), + outputs: Hex.from("02" + "e".repeat(64)), + locktime: Hex.from("00000000"), + } + + const deposit: DepositReceipt = { + depositor: EthereumAddress.from("0x" + "0".repeat(40)), + walletPublicKeyHash: Hex.from("ef5a2946f294f1742a779c9ac034bc3fa5d417b8"), + refundPublicKeyHash: Hex.from("b4f19a044feea3aa4a7d3f494433a11d0f1c400e"), + blindingFactor: Hex.from("b3460f26eda61ad1"), + refundLocktime: Hex.from("a1faa569"), + extraData: Hex.from(testAddress), + } + + await depositor.initializeDeposit(depositTx, 0, deposit) + expect(consoleLogStub.called).to.be.false + } finally { + consoleLogStub.restore() + } })🤖 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 245 - 280, The test "should not log reveal request payloads" currently stubs console.log into consoleLogStub and restores it only at the end, which can leak if the test throws; wrap the body that uses consoleLogStub and calls depositor.initializeDeposit(...) in a try/finally so that consoleLogStub.restore() is invoked in the finally block (keeping the assert expect(consoleLogStub.called).to.be.false inside the try or move it before restore), ensuring console.log is always restored even on failure.
🤖 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/README.md`:
- Around line 1685-1688: The README's "Defined in" source link is stale (points
to lib/starknet/index.ts:95) and must be regenerated so it matches the current
source (should point to lib/starknet/index.ts:92); run the TypeScript docs
generation script used by the repo (the docs build/generation command that
updates typescript/api-reference/README.md), commit the regenerated README so
the "Defined in" link for the StarkNet export in README reflects
lib/starknet/index.ts:92, and re-run the CI checks.
In `@typescript/src/lib/solana/solana-depositor-interface.ts`:
- Around line 74-82: The relayer POST call using
axios.post("https://relayer.tbtcscan.com/api/reveal", ...) must include a
per-request timeout to avoid hanging; update the axios.post invocation in the
SolanaDepositorInterface (the code that constructs
fundingTx/reveal/l2DepositOwner/l2Sender) to pass a third argument with a
timeout (e.g., { timeout: 10000 }) or use a shared configurable constant, and
ensure any timeout error is handled consistently with existing error handling in
that method.
In `@typescript/src/lib/starknet/index.ts`:
- Around line 52-79: validateProviderChain currently accepts alias chain IDs but
the code continues to use the original chainId for lookups (e.g.,
TBTC_CONTRACT_ADDRESSES) and config; this can pass validation but fail contract
resolution. Change the call to capture and use the canonical/normalized chain id
(e.g., const canonicalChainId = await validateProviderChain(provider, chainId))
and then use canonicalChainId everywhere downstream: build
depositorConfig.chainId = canonicalChainId, tokenConfig.chainId =
canonicalChainId, and use canonicalChainId when indexing TBTC_CONTRACT_ADDRESSES
before constructing StarkNetBitcoinDepositor and StarkNetTBTCTokenConfig; if
validateProviderChain does not currently return the canonical id, update it to
return the normalized id or add a helper normalizeChainId(provider, chainId) to
obtain it and use that value consistently.
In `@typescript/src/lib/sui/bitcoin-depositor.ts`:
- Around line 59-60: The exported setter setDepositOwner currently types its
parameter as ChainIdentifier | undefined but its implementation accepts null;
update the public signature to accept null as well (e.g., change the parameter
type to ChainIdentifier | undefined | null or ChainIdentifier | null) so callers
can pass null to clear the owner, and update any related type annotations or
exports referencing setDepositOwner to match the new nullable type; ensure
usages/call sites compile and adjust tests/types if necessary.
- Around line 90-92: The decode of deposit.extraData is happening outside the
initializeDeposit error handling and can throw a raw Error; move or wrap the
call to this.#extraDataEncoder.decodeDepositOwner(deposit.extraData) inside the
existing try/catch in initializeDeposit (or add a small try/catch around the
call) and on failure throw a SuiError with a clear message (include the original
error as cause or message) so that failures from decodeDepositOwner are surfaced
as SuiError like the rest of initializeDeposit; update the assignment to
depositOwner to use the decoded value only after successful decoding, e.g., by
referencing decodeDepositOwner within the protected block and falling back to
this.#depositOwner when appropriate.
In `@typescript/src/services/tbtc.ts`:
- Around line 293-295: The switch on l2ChainName has const declarations inside
case "StarkNet"/"Sui"/"Solana" which violates the noSwitchDeclarations rule and
can cause TDZ issues; fix it by wrapping each case body in its own block (case
"StarkNet": { ... break; }) so consts like the ones used with
TBTC.extractStarkNetAddress/starknetProvider are declared in block scope,
ensuring each case's variables are isolated and the linter error is resolved.
---
Outside diff comments:
In `@typescript/src/lib/starknet/starknet-depositor.ts`:
- Around line 111-121: The chain lookup in chainNameMap using config.chainId
fails for canonical alias inputs like "SN_MAIN"; update the logic so aliases are
accepted by either adding alias keys ("SN_GOERLI", "SN_SEPOLIA", "SN_MAIN") to
chainNameMap mapping to the same values as their hex counterparts, or normalize
config.chainId before lookup (e.g., translate known aliases to their hex form)
so the existing lookup for chainName succeeds; adjust the code that computes
chainName (variable chainName and map chainNameMap) accordingly to ensure both
hex IDs and canonical alias strings resolve to "StarknetMainnet" or
"StarknetTestnet" as intended.
---
Nitpick comments:
In `@typescript/test/lib/starknet/starknet-depositor-payload.test.ts`:
- Around line 245-280: The test "should not log reveal request payloads"
currently stubs console.log into consoleLogStub and restores it only at the end,
which can leak if the test throws; wrap the body that uses consoleLogStub and
calls depositor.initializeDeposit(...) in a try/finally so that
consoleLogStub.restore() is invoked in the finally block (keeping the assert
expect(consoleLogStub.called).to.be.false inside the try or move it before
restore), ensuring console.log is always restored even on failure.
🪄 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: d6b63ef4-01af-47cb-8a9c-06cb2783ee92
📒 Files selected for processing (25)
typescript/api-reference/README.mdtypescript/api-reference/classes/ElectrumClient.mdtypescript/api-reference/classes/StarkNetAddress.mdtypescript/api-reference/classes/StarkNetBitcoinDepositor.mdtypescript/api-reference/classes/TBTC.mdtypescript/api-reference/interfaces/StarkNetBitcoinDepositorConfig.mdtypescript/src/lib/electrum/client.tstypescript/src/lib/solana/index.tstypescript/src/lib/solana/solana-depositor-interface.tstypescript/src/lib/starknet/address.tstypescript/src/lib/starknet/index.tstypescript/src/lib/starknet/starknet-depositor.tstypescript/src/lib/sui/bitcoin-depositor.tstypescript/src/services/tbtc.tstypescript/test/lib/electrum.test.tstypescript/test/lib/solana/index.test.tstypescript/test/lib/starknet/address.test.tstypescript/test/lib/starknet/extra-data-encoder.test.tstypescript/test/lib/starknet/index.test.tstypescript/test/lib/starknet/integration.test.tstypescript/test/lib/starknet/module-integration.test.tstypescript/test/lib/starknet/starknet-depositor-payload.test.tstypescript/test/lib/starknet/test-helpers.tstypescript/test/lib/sui/bitcoin-depositor.test.tstypescript/test/services/tbtc-starknet-address.test.ts
Comment on lines
74
to
82
| const response = await axios.post( | ||
| "http://relayer.tbtcscan.com/api/reveal", | ||
| "https://relayer.tbtcscan.com/api/reveal", | ||
| { | ||
| fundingTx, | ||
| reveal, | ||
| l2DepositOwner: extraData, | ||
| l2Sender: `0x${this.#depositOwner?.identifierHex}`, | ||
| l2Sender: `0x${this.#depositOwner.identifierHex}`, | ||
| } | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant section in the target file
sed -n '1,140p' typescript/src/lib/solana/solana-depositor-interface.ts | nl -ba | sed -n '60,110p'
# Search for axios.post usages around the relayer URL / file
rg -n "axios\.post\(|relayer\.tbtcscan\.com|api/reveal|timeout" typescript/src/lib/solana/solana-depositor-interface.ts typescript/src/lib -S
# Find axios configuration / default timeout patterns in the repo
rg -n "axios\.create\(|defaults\.timeout|timeout\s*:" typescript/src -SRepository: threshold-network/tbtc-v2
Length of output: 112
🏁 Script executed:
#!/bin/bash
set +euo pipefail
FILE="typescript/src/lib/solana/solana-depositor-interface.ts"
# Show the relevant section with line numbers via awk
awk 'NR>=60 && NR<=110 {printf "%4d\t%s\n", NR, $0}' "$FILE"
echo "---- axios.post / relayer usage in solana-depositor-interface.ts ----"
rg -n "axios\.post\(|relayer\.tbtcscan\.com|api/reveal|timeout" "$FILE" -S
echo "---- axios configuration / defaults across repo (axios.create, defaults.timeout, timeout:) ----"
rg -n "axios\.create\(|axios\.defaults\.(timeout|common)|defaults\.timeout|timeout\s*:" typescript/src -SRepository: threshold-network/tbtc-v2
Length of output: 2282
🏁 Script executed:
#!/bin/bash
set +euo pipefail
FILE="typescript/src/lib/solana/solana-depositor-interface.ts"
echo "---- axios import / config in target file ----"
rg -n "import .*axios|axios\.defaults|axios\.create" "$FILE" -S
echo "---- top of file (imports section) ----"
head -n 60 "$FILE" | nl -ba | sed -n '1,60p' || head -n 60 "$FILE"
echo "---- repo-wide axios defaults / create ----"
rg -n "axios\.defaults\.timeout|axios\.defaults|axios\.create\(|axios\.post\(" typescript/src -SRepository: threshold-network/tbtc-v2
Length of output: 387
🏁 Script executed:
#!/bin/bash
set +euo pipefail
FILE="typescript/src/lib/solana/solana-depositor-interface.ts"
echo "---- timeout occurrences in target file ----"
rg -n "timeout" "$FILE" -S || true
echo "---- axios.post calls passing timeout option (repo-wide) ----"
rg -n "axios\.post\([^\\)]*(?:timeout|timeout\s*:)" typescript/src -S || true
echo "---- axios.defaults or axios.create occurrences (repo-wide) ----"
rg -n "axios\.defaults|axios\.create" typescript/src -S || trueRepository: threshold-network/tbtc-v2
Length of output: 241
Add an explicit timeout to the relayer POST call.
axios.post("https://relayer.tbtcscan.com/api/reveal", ...) is made without a per-request timeout (no axios.defaults.timeout / axios.create-level timeout found), so it can hang indefinitely during network/relayer degradation.
Suggested fix
const response = await axios.post(
"https://relayer.tbtcscan.com/api/reveal",
{
fundingTx,
reveal,
l2DepositOwner: extraData,
l2Sender: `0x${this.#depositOwner.identifierHex}`,
- }
+ },
+ {
+ timeout: 15_000,
+ }
)🤖 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/src/lib/solana/solana-depositor-interface.ts` around lines 74 -
82, The relayer POST call using
axios.post("https://relayer.tbtcscan.com/api/reveal", ...) must include a
per-request timeout to avoid hanging; update the axios.post invocation in the
SolanaDepositorInterface (the code that constructs
fundingTx/reveal/l2DepositOwner/l2Sender) to pass a third argument with a
timeout (e.g., { timeout: 10000 }) or use a shared configurable constant, and
ensure any timeout error is handled consistently with existing error handling in
that method.
Comment on lines
+52
to
+79
| await validateProviderChain(provider, chainId) | ||
|
|
||
| // Build depositor configuration with environment variable support | ||
| const depositorConfig: StarkNetBitcoinDepositorConfig = { | ||
| chainId, | ||
| relayerUrl: process.env.STARKNET_RELAYER_URL, // Optional override | ||
| defaultVault: process.env.STARKNET_TBTC_VAULT, // Optional override | ||
| } | ||
|
|
||
| // Create provider if not provided (for testing/backward compatibility) | ||
| const actualProvider = provider || createMockProvider() | ||
|
|
||
| // Create the main depositor instance | ||
| const starkNetBitcoinDepositor = new StarkNetBitcoinDepositor( | ||
| depositorConfig, | ||
| "StarkNet", | ||
| actualProvider | ||
| provider | ||
| ) | ||
|
|
||
| // Set the deposit owner | ||
| starkNetBitcoinDepositor.setDepositOwner(StarkNetAddress.from(walletAddress)) | ||
|
|
||
| // Create token instance | ||
| let starkNetTbtcToken: StarkNetTBTCToken | ||
|
|
||
| if (provider) { | ||
| // Provider available - create full implementation with balance queries | ||
| const tokenContract = TBTC_CONTRACT_ADDRESSES[chainId] | ||
| if (!tokenContract) { | ||
| throw new Error(`No tBTC contract address for chain ${chainId}`) | ||
| } | ||
|
|
||
| const tokenConfig: StarkNetTBTCTokenConfig = { | ||
| chainId, | ||
| tokenContract, | ||
| } | ||
|
|
||
| starkNetTbtcToken = new StarkNetTBTCToken(tokenConfig, provider) | ||
| } else { | ||
| // No provider - create interface-only implementation | ||
| // This maintains backward compatibility for tests | ||
| const mockConfig: StarkNetTBTCTokenConfig = { | ||
| chainId, | ||
| tokenContract: "0x0", // Placeholder | ||
| } | ||
|
|
||
| // Create a mock provider that throws errors | ||
| const mockProvider = { | ||
| getChainId: () => Promise.resolve(chainId), | ||
| } as any | ||
|
|
||
| starkNetTbtcToken = new StarkNetTBTCToken(mockConfig, mockProvider) | ||
| const tokenContract = TBTC_CONTRACT_ADDRESSES[chainId] | ||
| if (!tokenContract) { | ||
| throw new Error(`No tBTC contract address for chain ${chainId}`) | ||
| } | ||
|
|
||
| const tokenConfig: StarkNetTBTCTokenConfig = { | ||
| chainId, | ||
| tokenContract, | ||
| } |
There was a problem hiding this comment.
Normalize chainId once and use the canonical value for downstream config/lookups.
validateProviderChain accepts alias chain IDs, but TBTC_CONTRACT_ADDRESSES[chainId] and config still use the raw value. That can pass validation and then fail contract resolution for alias inputs.
Suggested fix
export async function loadStarkNetCrossChainInterfaces(
walletAddress: string,
provider: StarkNetProvider,
chainId: string = Chains.StarkNet.Sepolia
): Promise<DestinationChainInterfaces> {
if (!provider) {
throw new Error("StarkNet provider is required")
}
- await validateProviderChain(provider, chainId)
+ const normalizedChainId = normalizeStarkNetChainId(chainId)
+ await validateProviderChain(provider, normalizedChainId)
// Build depositor configuration with environment variable support
const depositorConfig: StarkNetBitcoinDepositorConfig = {
- chainId,
+ chainId: normalizedChainId,
relayerUrl: process.env.STARKNET_RELAYER_URL, // Optional override
defaultVault: process.env.STARKNET_TBTC_VAULT, // Optional override
}
@@
- const tokenContract = TBTC_CONTRACT_ADDRESSES[chainId]
+ const tokenContract = TBTC_CONTRACT_ADDRESSES[normalizedChainId]
if (!tokenContract) {
- throw new Error(`No tBTC contract address for chain ${chainId}`)
+ throw new Error(`No tBTC contract address for chain ${normalizedChainId}`)
}
const tokenConfig: StarkNetTBTCTokenConfig = {
- chainId,
+ chainId: normalizedChainId,
tokenContract,
}Also applies to: 126-134
🧰 Tools
🪛 GitHub Actions: TypeScript bindings / 1_typescript-build-and-test.txt
[error] 52-52: Failed to load StarkNet cross-chain interfaces because starknet_chainId RPC call failed.
🪛 GitHub Actions: TypeScript bindings / typescript-build-and-test
[error] 52-110: RPC failed during StarkNet chain id resolution: RpcError: RPC: starknet_chainId with params undefined; -32000: Blast API is no longer available. Please update your integration to use Alchemy's API instead.
🤖 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/src/lib/starknet/index.ts` around lines 52 - 79,
validateProviderChain currently accepts alias chain IDs but the code continues
to use the original chainId for lookups (e.g., TBTC_CONTRACT_ADDRESSES) and
config; this can pass validation but fail contract resolution. Change the call
to capture and use the canonical/normalized chain id (e.g., const
canonicalChainId = await validateProviderChain(provider, chainId)) and then use
canonicalChainId everywhere downstream: build depositorConfig.chainId =
canonicalChainId, tokenConfig.chainId = canonicalChainId, and use
canonicalChainId when indexing TBTC_CONTRACT_ADDRESSES before constructing
StarkNetBitcoinDepositor and StarkNetTBTCTokenConfig; if validateProviderChain
does not currently return the canonical id, update it to return the normalized
id or add a helper normalizeChainId(provider, chainId) to obtain it and use that
value consistently.
Comment on lines
+59
to
+60
| setDepositOwner(depositOwner: ChainIdentifier | undefined): void { | ||
| if (depositOwner === undefined || depositOwner === null) { |
There was a problem hiding this comment.
Include null in the exported setter type.
The body treats null as a supported way to clear the owner, but the public signature only accepts undefined. That means TS consumers still can't use the nullable API this change is trying to expose.
Suggested fix
- setDepositOwner(depositOwner: ChainIdentifier | undefined): void {
+ setDepositOwner(depositOwner: ChainIdentifier | null | undefined): void {📝 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.
Suggested change
| setDepositOwner(depositOwner: ChainIdentifier | undefined): void { | |
| if (depositOwner === undefined || depositOwner === null) { | |
| setDepositOwner(depositOwner: ChainIdentifier | null | undefined): void { | |
| if (depositOwner === undefined || depositOwner === null) { |
🤖 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/src/lib/sui/bitcoin-depositor.ts` around lines 59 - 60, The
exported setter setDepositOwner currently types its parameter as ChainIdentifier
| undefined but its implementation accepts null; update the public signature to
accept null as well (e.g., change the parameter type to ChainIdentifier |
undefined | null or ChainIdentifier | null) so callers can pass null to clear
the owner, and update any related type annotations or exports referencing
setDepositOwner to match the new nullable type; ensure usages/call sites compile
and adjust tests/types if necessary.
Comment on lines
+90
to
+92
| const depositOwner = deposit.extraData | ||
| ? this.#extraDataEncoder.decodeDepositOwner(deposit.extraData) | ||
| : this.#depositOwner |
There was a problem hiding this comment.
Wrap malformed extraData decode failures in SuiError.
decodeDepositOwner(...) runs before the existing try/catch, so invalid external deposit.extraData escapes as a raw Error instead of the typed SuiError used for the rest of initializeDeposit.
Suggested fix
- const depositOwner = deposit.extraData
- ? this.#extraDataEncoder.decodeDepositOwner(deposit.extraData)
- : this.#depositOwner
+ let depositOwner = this.#depositOwner
+ if (deposit.extraData) {
+ try {
+ depositOwner = this.#extraDataEncoder.decodeDepositOwner(
+ deposit.extraData
+ )
+ } catch (error) {
+ throw new SuiError("Invalid SUI deposit owner in extra data", error)
+ }
+ }🤖 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/src/lib/sui/bitcoin-depositor.ts` around lines 90 - 92, The decode
of deposit.extraData is happening outside the initializeDeposit error handling
and can throw a raw Error; move or wrap the call to
this.#extraDataEncoder.decodeDepositOwner(deposit.extraData) inside the existing
try/catch in initializeDeposit (or add a small try/catch around the call) and on
failure throw a SuiError with a clear message (include the original error as
cause or message) so that failures from decodeDepositOwner are surfaced as
SuiError like the rest of initializeDeposit; update the assignment to
depositOwner to use the decoded value only after successful decoding, e.g., by
referencing decodeDepositOwner within the protected block and falling back to
this.#depositOwner when appropriate.
Comment on lines
+293
to
+295
| const walletAddressHex = await TBTC.extractStarkNetAddress( | ||
| starknetProvider | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="typescript/src/services/tbtc.ts"
# Show the referenced area and a bit more context to understand the surrounding switch/case.
sed -n '240,380p' "$FILE" | nl -ba | sed -n '1,200p'
# Specifically locate the switch and the StarkNet/Sui/Solana cases (in case line numbers drift).
rg -n --context 3 'switch\s*\(' "$FILE"
rg -n 'case\s+"(StarkNet|Sui|Solana)"' "$FILE" --context 5
# Search for the other referenced lines region.
sed -n '300,360p' "$FILE" | nl -ba
# If eslint plugin is configured in repo, locate the rule name to confirm intended lint behavior.
rg -n 'noSwitchDeclarations' -S .Repository: threshold-network/tbtc-v2
Length of output: 112
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="typescript/src/services/tbtc.ts"
echo "== Context around lines ~250-380 =="
awk 'NR>=240 && NR<=380 {printf "%5d %s\n", NR, $0}' "$FILE"
echo
echo "== Cases for StarkNet/Sui/Solana =="
rg -n 'case\s+"(StarkNet|Sui|Solana)"' "$FILE" -n --context 8
echo
echo "== Find any switch() in this file =="
rg -n 'switch\s*\(' "$FILE"
echo
echo "== Confirm lint rule presence (search for noSwitchDeclarations) =="
rg -n 'noSwitchDeclarations' -S . || trueRepository: threshold-network/tbtc-v2
Length of output: 7519
Block-scope switch case const declarations for l2ChainName (StarkNet/Sui/Solana)—Biome noSwitchDeclarations is set to error.
typescript/src/services/tbtc.ts declares const directly inside case "StarkNet", case "Sui", and case "Solana" without wrapping each case body in { ... }, which both triggers the configured lint rule and risks switch-scope/TDZ hazards.
🧰 Tools
🪛 Biome (2.4.15)
[error] 293-295: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
(lint/correctness/noSwitchDeclarations)
🪛 GitHub Actions: TypeScript bindings / 1_typescript-build-and-test.txt
[error] 293-293: TBTC.initializeCrossChain failed because extractStarkNetAddress rejected the provided StarkNet provider.
🤖 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/src/services/tbtc.ts` around lines 293 - 295, The switch on
l2ChainName has const declarations inside case "StarkNet"/"Sui"/"Solana" which
violates the noSwitchDeclarations rule and can cause TDZ issues; fix it by
wrapping each case body in its own block (case "StarkNet": { ... break; }) so
consts like the ones used with TBTC.extractStarkNetAddress/starknetProvider are
declared in block scope, ensuring each case's variables are isolated and the
linter error is resolved.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
typescript/test/services/tbtc-starknet.test.ts (1)
65-76:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTest intent and assertion diverge for
_l2Signer.At Line 65, the test says it verifies
_l2Signerstorage, but it only asserts contracts exist. This won’t catch_l2Signerregressions; either rename the test or assert the expected_l2Signerstate explicitly.Suggested fix
- it("should store StarkNet provider in _l2Signer property", async () => { + it("should initialize StarkNet contracts in single-parameter mode", async () => { const starknetProvider = createMockProvider() // Act await tbtc.initializeCrossChain("StarkNet", starknetProvider as any) // Assert - check internal _l2Signer property - // Note: This would require making _l2Signer accessible for testing - // or using a getter method const contracts = tbtc.crossChainContracts("StarkNet") expect(contracts).to.not.be.undefined + expect((tbtc as any)._l2Signer).to.be.undefined })🤖 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/services/tbtc-starknet.test.ts` around lines 65 - 76, The test name claims to verify storage of the internal _l2Signer but only checks crossChainContracts; update the test to either (A) rename it to reflect that it only asserts contracts exist, or (B) assert the actual _l2Signer is set by exposing it via a test accessor/getter or using an existing method: call tbtc.initializeCrossChain("StarkNet", starknetProvider) and then read/assert tbtc._l2Signer (or tbtc.getL2Signer()/equivalent) equals the provided provider/mock; if no accessor exists add a minimal test-only getter that returns _l2Signer so the test can assert identity/instance rather than only checking crossChainContracts("StarkNet").typescript/test/lib/starknet/provider-integration.test.ts (1)
52-77:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix async assertion in
provider-integration.test.tsdeposit-flow test
expect(async () => { ... }).to.not.throw()won’t reliably fail on async rejection (the throw check is sync-oriented). Use the repo’s promise matcher style instead (await expect(promise).to.not.be.rejected).Suggested change
- // Act & Assert - should not throw - expect(async () => { - await tbtc.initializeCrossChain("StarkNet", starknetProvider) - // Verify contracts were created (not _l2Signer) - const contracts = tbtc.crossChainContracts("StarkNet") - expect(contracts).to.exist - }).to.not.throw() + await expect( + tbtc.initializeCrossChain("StarkNet", starknetProvider) + ).to.not.be.rejected + + // Verify contracts were created (not _l2Signer) + const contracts = tbtc.crossChainContracts("StarkNet") + expect(contracts).to.exist🤖 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/provider-integration.test.ts` around lines 52 - 77, The test uses expect(async () => { ... }).to.not.throw(), which doesn't catch async rejections; instead call the async flow as a promise and use the promise matcher: create a promise that runs await tbtc.initializeCrossChain("StarkNet", starknetProvider) and then checks tbtc.crossChainContracts("StarkNet") exists, then assert with await expect(promise).to.not.be.rejected (or await expect(promise).to.eventually.be.fulfilled depending on chai setup); replace the current expect wrapper around the async arrow with this promise-based assertion and keep references to initializeCrossChain and crossChainContracts so the test verifies the same behavior.
🤖 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 `@typescript/test/lib/starknet/provider-integration.test.ts`:
- Around line 52-77: The test uses expect(async () => { ... }).to.not.throw(),
which doesn't catch async rejections; instead call the async flow as a promise
and use the promise matcher: create a promise that runs await
tbtc.initializeCrossChain("StarkNet", starknetProvider) and then checks
tbtc.crossChainContracts("StarkNet") exists, then assert with await
expect(promise).to.not.be.rejected (or await
expect(promise).to.eventually.be.fulfilled depending on chai setup); replace the
current expect wrapper around the async arrow with this promise-based assertion
and keep references to initializeCrossChain and crossChainContracts so the test
verifies the same behavior.
In `@typescript/test/services/tbtc-starknet.test.ts`:
- Around line 65-76: The test name claims to verify storage of the internal
_l2Signer but only checks crossChainContracts; update the test to either (A)
rename it to reflect that it only asserts contracts exist, or (B) assert the
actual _l2Signer is set by exposing it via a test accessor/getter or using an
existing method: call tbtc.initializeCrossChain("StarkNet", starknetProvider)
and then read/assert tbtc._l2Signer (or tbtc.getL2Signer()/equivalent) equals
the provided provider/mock; if no accessor exists add a minimal test-only getter
that returns _l2Signer so the test can assert identity/instance rather than only
checking crossChainContracts("StarkNet").
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1dfa990b-0bd4-48d8-a58a-777ec279fefb
📒 Files selected for processing (10)
typescript/api-reference/README.mdtypescript/test/integration/starknet-single-param-flow.test.tstypescript/test/lib/starknet/provider-integration.test.tstypescript/test/lib/starknet/starknet-depositor-implementation.test.tstypescript/test/lib/starknet/starknet-depositor.test.tstypescript/test/services/tbtc-refactored-init.test.tstypescript/test/services/tbtc-single-param.test.tstypescript/test/services/tbtc-starknet.test.tstypescript/test/services/tbtc-t003-l2signer.test.tstypescript/test/services/threshold-context-provider-compatibility.test.ts
✅ Files skipped from review due to trivial changes (2)
- typescript/test/lib/starknet/starknet-depositor-implementation.test.ts
- typescript/test/lib/starknet/starknet-depositor.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- typescript/api-reference/README.md
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.
Summary
Validation
git diff --checkpassed locally for the safe-fix batchNotes
This is public-safe client hardening only and does not include any private deployment mitigation. StarkNet provider-chain binding remains a follow-up because the first implementation made existing tests depend on a retired live RPC endpoint.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests