feat: implement useContractWrite hook for standardized Soroban write …#145
feat: implement useContractWrite hook for standardized Soroban write …#145soma-enyi wants to merge 2 commits intoTheBlockCade:mainfrom
Conversation
…execution - Add useContractWrite hook with deterministic state transitions (idle/loading/success/error) - Implement wallet and network precondition validation - Support optional idempotency key attachment for duplicate prevention - Integrate with error mapping service for normalized error handling - Add contract method registry for type-safe method execution - Include comprehensive test suite with 11 passing tests - Add detailed documentation with usage examples and best practices Closes #[issue-number]
📝 WalkthroughWalkthroughThis PR introduces Changes
Sequence DiagramsequenceDiagram
participant Component
participant Hook as useContractWrite
participant WalletSession
participant NetworkGuard
participant Registry as Method Registry
participant Mapper as Error Mapper
Component->>Hook: write(method, params, options)
activate Hook
Hook->>WalletSession: Check wallet connected
alt Wallet not connected
WalletSession-->>Hook: Error
Hook->>Mapper: mapRpcError()
Mapper-->>Hook: AppError
Hook-->>Component: phase: error
else Wallet connected
Hook->>NetworkGuard: Validate network support
alt Network unsupported
NetworkGuard-->>Hook: Error
Hook->>Mapper: mapRpcError()
Mapper-->>Hook: AppError
Hook-->>Component: phase: error
else Network supported
Hook-->>Component: phase: loading
Hook->>Registry: executeContractMethod(method, params)
alt Method execution succeeds
Registry-->>Hook: {data, txHash, confirmations}
Hook-->>Component: phase: success, data, txHash
else Method execution fails
Registry-->>Hook: Error
Hook->>Mapper: mapRpcError()
Mapper-->>Hook: AppError
Hook-->>Component: phase: error, error
end
end
end
deactivate Hook
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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.
Actionable comments posted: 8
🧹 Nitpick comments (5)
frontend/src/hooks/v1/useContractWrite.ts (2)
173-175:reset()does not clear the idempotency key registry.After calling
reset(),idempotencyKeysRef.currentstill contains all previously-seen keys. A component that resets and retries the same logical operation with the same idempotency key will spuriously trigger the duplicate-key warning even though the hook is logically back in idle state.♻️ Proposed fix
const reset = useCallback(() => { setState({ phase: 'idle', confirmations: 0 }); + idempotencyKeysRef.current.clear(); }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/hooks/v1/useContractWrite.ts` around lines 173 - 175, The reset() callback currently only resets state but leaves idempotencyKeysRef.current populated, causing stale duplicate-key warnings; update reset (the useCallback defined as const reset = useCallback(...)) to also clear the idempotency key registry by resetting idempotencyKeysRef.current (e.g., call idempotencyKeysRef.current.clear() or assign a new Set()) so the hook truly returns to an idle, fresh state.
170-171:networkGuardobject inuseCallbackdeps causeswriteto rememoize every render.
useNetworkGuardreturns a new object reference on every call, so includingnetworkGuardin the dependency array meanswriteis recreated on every render, defeating memoization. Only the stable method extracted from it is needed.♻️ Proposed fix
- const networkGuard = useNetworkGuard({ network: walletMeta?.network }); + const { assertSupportedNetwork } = useNetworkGuard({ network: walletMeta?.network }); ... - const networkCheck = networkGuard.assertSupportedNetwork(); + const networkCheck = assertSupportedNetwork(); ... - [client, walletState, walletMeta, networkGuard], + [client, walletState, walletMeta, assertSupportedNetwork],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/hooks/v1/useContractWrite.ts` around lines 170 - 171, The write callback is being rememoized every render because the entire networkGuard object is in the useCallback dependency array; extract the specific method from networkGuard that write actually uses (the stable function returned by useNetworkGuard), use that extracted function inside write, remove the networkGuard object from the dependency array, and include only that stable method (and other real primitives like client, walletState, walletMeta) so write remains memoized.frontend/src/types/contracts/write.ts (1)
37-37:params?: any[]silently disables type-checking on write arguments.Prefer
unknown[]to retain compile-time safety at call sites. Any code passing mistyped params will still compile withany[].♻️ Proposed fix
- write: (method: string, params?: any[], options?: ContractWriteOptions) => Promise<TData>; + write: (method: string, params?: unknown[], options?: ContractWriteOptions) => Promise<TData>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/types/contracts/write.ts` at line 37, The write method signature uses params?: any[] which disables compile-time checking; change the signature in the Contract write type from params?: any[] to params?: unknown[] (i.e., update the method declaration write: (method: string, params?: unknown[], options?: ContractWriteOptions) => Promise<TData>), and then update any implementations/call-sites that relied on implicit any to explicitly cast or validate params as needed so callers remain type-safe.frontend/src/utils/v1/contractMethodRegistry.ts (1)
15-43: Registry entries typeclientasany, losing all type-safety for client method dispatch.Every entry in
CONTRACT_METHOD_REGISTRYuses(client: any, params: any[]). SinceSorobanContractClientis already imported at Line 8, the entries should use it directly. This also means that typos in method names (e.g.,client.pool_resrve(...)) are silently ignored by the compiler.♻️ Proposed refactor
- 'pool_fund': (client: any, params: any[]) => + 'pool_fund': (client: SorobanContractClient, params: unknown[]) => client.pool_fund(params[0], params[1], params[2]),Apply to all registry entries.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/utils/v1/contractMethodRegistry.ts` around lines 15 - 43, The registry currently types the client parameter as any which loses compile-time checking; update every entry in CONTRACT_METHOD_REGISTRY to use the concrete SorobanContractClient type (replace (client: any, params: any[]) with (client: SorobanContractClient, params: unknown[] or a more specific params tuple) so method names are checked by the compiler), ensure SorobanContractClient is imported where this file references it, apply this change across all entries (e.g., pool_fund, pool_reserve, pool_release, pool_payout, badge_define, badge_award, access_grantRole, access_revokeRole, coinflip_placeBet, coinflip_resolveBet) and run TS build to catch any typos in method names and fix them accordingly.frontend/tests/hooks/v1/useContractWrite.test.ts (1)
232-259: Missing test:reset()should also clear the idempotency key registry.The reset test verifies state fields are cleared but does not assert that idempotency keys are cleared. This means the currently-undocumented behaviour (keys survive reset) is not explicitly tested. Once the suggested fix to
reset()is applied (clearingidempotencyKeysRef), a test like the one below should be added to guard the contract:it('should clear idempotency keys on reset', async () => { const { result } = renderHook(() => useContractWrite()); const key = 'reuse-after-reset'; const consoleSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); // First call consumes the key await act(async () => { try { await result.current.write('test_method', [], { idempotencyKey: key }); } catch {} }); // Reset await act(async () => { result.current.reset(); }); // Second call — key should be fresh, no warning expected await act(async () => { try { await result.current.write('test_method', [], { idempotencyKey: key }); } catch {} }); expect(consoleSpy).not.toHaveBeenCalled(); consoleSpy.mockRestore(); });Would you like me to open a new issue to track this test addition?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/tests/hooks/v1/useContractWrite.test.ts` around lines 232 - 259, The reset test must also verify that idempotency keys are cleared: update the useContractWrite.reset() implementation to clear the internal idempotencyKeysRef (reset idempotencyKeysRef.current = new Set() or equivalent) and add a new test in frontend/tests/hooks/v1/useContractWrite.test.ts that calls useContractWrite().write(..., { idempotencyKey: key }) once, calls result.current.reset(), then calls write with the same key again and asserts no warning occurred (use vi.spyOn(console, 'warn') to check); reference the reset() method, idempotencyKeysRef, useContractWrite(), and write() when making the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/hooks/v1/useContractWrite.README.md`:
- Around line 95-98: The fenced code block that renders the state diagram ("idle
→ loading → success ↘ error") lacks a language tag and triggers MD040; update
that block by adding a language specifier such as ```text or ```plaintext
immediately after the opening backticks so the block becomes fenced with a
language identifier (e.g., change the opening ``` to ```text) to silence the
markdownlint warning.
- Around line 120-127: The example uses an invalid TypeScript catch annotation
`catch (err: AppError)` for the `write` call; change the catch to a plain `catch
(err)` and then narrow or cast the error inside the block (e.g., `if (err
instanceof AppError) { ... }` or `const appErr = err as AppError`) before
accessing `err.code`, `err.domain`, and `err.message`, updating the example
around the `write('method_name', [param1, param2])` snippet to reflect the valid
pattern.
In `@frontend/src/hooks/v1/useContractWrite.ts`:
- Around line 154-159: The error context currently passes a raw Stellar public
key (walletMeta?.publicKey) into mapRpcError which can leak PII; update the
catch block in useContractWrite to redact before forwarding: create a redacted
value (e.g., hashed or truncated/prefixed such as first 8 chars + ellipsis) and
pass that as the wallet field to mapRpcError (replace walletMeta?.publicKey with
the redacted variable), or omit the wallet field entirely if not needed for
debugging; ensure the redaction logic runs locally in the catch block so only
the safe token is sent to mappedError.
- Around line 113-134: The idempotency key is being consumed before validating
the contract method, causing invalid calls to permanently register keys; move
the idempotency handling block that checks and adds options.idempotencyKey (uses
idempotencyKeysRef.current.has(...) and .add(...)) to after the method
validation (the if (!method || typeof method !== 'string') branch where
setState({ phase: 'error', ... }) and throw error occur) so keys are only
recorded for requests that pass validation; ensure the duplicate warning logic
remains the same but executes after the method check.
In `@frontend/src/utils/v1/contractMethodRegistry.ts`:
- Around line 53-82: executeContractMethod currently accepts options?:
ContractWriteOptions but never uses confirmationTimeoutMs or pollIntervalMs;
either implement polling using those values or remove the parameter. Fix by
adding post-execution confirmation polling after calling executor(client,
params): if result.ledger is undefined (or confirmations are required),
repeatedly call client.getTransaction or the executor's status-check helper
(referencing executeContractMethod, CONTRACT_METHOD_REGISTRY, and
SorobanContractClient) at intervals defined by options.pollIntervalMs until
either confirmations are observed or options.confirmationTimeoutMs elapses, then
return data/txHash/confirmations; ensure default timeout/interval fallbacks are
applied when options is absent.
- Around line 58-81: The returned object from the executor includes an extra
property "ledger" that isn't in the declared return type of the function,
causing a TypeScript excess-property error; remove "ledger: result.ledger" from
the returned object (or alternatively expand the Promise return type to include
ledger if you intend to return it) and keep the returned shape consistent with
Promise<{ data: TData; txHash?: string; confirmations?: number }>. Also update
the consumer in useContractWrite (the hook using result.confirmations) to stop
using the falsy-coalescing override "?? 1" which converts 0 to 1; instead use an
explicit undefined check (e.g., confirmations === undefined ? 1 : confirmations)
or simply propagate the confirmations value so that a 0 confirmation remains 0.
Ensure changes reference the executor invocation and the
CONTRACT_METHOD_REGISTRY lookup so the returned shape and the hook's handling
stay aligned.
In `@frontend/tests/hooks/v1/useContractWrite.test.ts`:
- Around line 248-250: The test calls act(() => { result.current.reset(); })
synchronously; change the test to await the act() call (e.g., make the test
async and use await act(() => result.current.reset())) so React's async act from
RTL v16 is properly awaited; update the test declaration to async and replace
the act invocation referencing act and result.current.reset accordingly.
- Around line 114-138: The hook useContractWrite incorrectly assumes
assertSupportedNetwork returns top-level fields like isSupported, message, and
actual; update the hook to treat the return as an AssertNetworkResult (check
result.ok rather than isSupported and read error?.message and error?.actual from
the nested error object), adjust any logic that sets phase or throws
WALLET_NETWORK_MISMATCH accordingly, and then update the test mock
(useNetworkGuardModule.useNetworkGuard) to return the real AssertNetworkResult
shape (e.g., { ok: false, error: { message: '...', actual: '...',
supportedNetworks: [...] } }) so the test no longer masks the type mismatch.
---
Nitpick comments:
In `@frontend/src/hooks/v1/useContractWrite.ts`:
- Around line 173-175: The reset() callback currently only resets state but
leaves idempotencyKeysRef.current populated, causing stale duplicate-key
warnings; update reset (the useCallback defined as const reset =
useCallback(...)) to also clear the idempotency key registry by resetting
idempotencyKeysRef.current (e.g., call idempotencyKeysRef.current.clear() or
assign a new Set()) so the hook truly returns to an idle, fresh state.
- Around line 170-171: The write callback is being rememoized every render
because the entire networkGuard object is in the useCallback dependency array;
extract the specific method from networkGuard that write actually uses (the
stable function returned by useNetworkGuard), use that extracted function inside
write, remove the networkGuard object from the dependency array, and include
only that stable method (and other real primitives like client, walletState,
walletMeta) so write remains memoized.
In `@frontend/src/types/contracts/write.ts`:
- Line 37: The write method signature uses params?: any[] which disables
compile-time checking; change the signature in the Contract write type from
params?: any[] to params?: unknown[] (i.e., update the method declaration write:
(method: string, params?: unknown[], options?: ContractWriteOptions) =>
Promise<TData>), and then update any implementations/call-sites that relied on
implicit any to explicitly cast or validate params as needed so callers remain
type-safe.
In `@frontend/src/utils/v1/contractMethodRegistry.ts`:
- Around line 15-43: The registry currently types the client parameter as any
which loses compile-time checking; update every entry in
CONTRACT_METHOD_REGISTRY to use the concrete SorobanContractClient type (replace
(client: any, params: any[]) with (client: SorobanContractClient, params:
unknown[] or a more specific params tuple) so method names are checked by the
compiler), ensure SorobanContractClient is imported where this file references
it, apply this change across all entries (e.g., pool_fund, pool_reserve,
pool_release, pool_payout, badge_define, badge_award, access_grantRole,
access_revokeRole, coinflip_placeBet, coinflip_resolveBet) and run TS build to
catch any typos in method names and fix them accordingly.
In `@frontend/tests/hooks/v1/useContractWrite.test.ts`:
- Around line 232-259: The reset test must also verify that idempotency keys are
cleared: update the useContractWrite.reset() implementation to clear the
internal idempotencyKeysRef (reset idempotencyKeysRef.current = new Set() or
equivalent) and add a new test in
frontend/tests/hooks/v1/useContractWrite.test.ts that calls
useContractWrite().write(..., { idempotencyKey: key }) once, calls
result.current.reset(), then calls write with the same key again and asserts no
warning occurred (use vi.spyOn(console, 'warn') to check); reference the reset()
method, idempotencyKeysRef, useContractWrite(), and write() when making the
changes.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
frontend/src/hooks/v1/index.tsfrontend/src/hooks/v1/useContractWrite.README.mdfrontend/src/hooks/v1/useContractWrite.tsfrontend/src/types/contracts/write.tsfrontend/src/utils/v1/contractMethodRegistry.tsfrontend/tests/hooks/v1/useContractWrite.test.ts
| ``` | ||
| idle → loading → success | ||
| ↘ error | ||
| ``` |
There was a problem hiding this comment.
Add a language specifier to the state machine code block.
The fenced block on line 95 has no language tag, which triggers the MD040 markdownlint warning.
✏️ Proposed fix
-```
+```
idle → loading → success
↘ error
-```
+```Use ```text or ```plaintext as the identifier.
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 95-95: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/hooks/v1/useContractWrite.README.md` around lines 95 - 98, The
fenced code block that renders the state diagram ("idle → loading → success ↘
error") lacks a language tag and triggers MD040; update that block by adding a
language specifier such as ```text or ```plaintext immediately after the opening
backticks so the block becomes fenced with a language identifier (e.g., change
the opening ``` to ```text) to silence the markdownlint warning.
| try { | ||
| await write('method_name', [param1, param2]); | ||
| } catch (err: AppError) { | ||
| console.error(err.code); // Structured error code | ||
| console.error(err.domain); // Error domain (RPC, CONTRACT, WALLET) | ||
| console.error(err.message); // Human-readable message | ||
| } | ||
| ``` |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
TypeScript catch clause variable type annotation restrictions catch(err: CustomType)
💡 Result:
In TypeScript, the catch clause variable cannot be annotated with an arbitrary type like catch (err: CustomType). Historically TS disallowed any annotation there; since TS 4.0 it only allows annotating the catch variable as any or unknown (because anything can be thrown in JavaScript). [1][2]
So this is invalid:
try {
// ...
} catch (err: CustomType) { // ❌ not allowed
}Do this instead (recommended):
try {
// ...
} catch (err: unknown) {
if (err instanceof CustomError) {
// here err is CustomError
}
}Or opt out:
} catch (err: any) {
// ...
}Also note: TS 4.4 introduced --useUnknownInCatchVariables, which changes the default type of err from any to unknown (and it’s enabled when strict is on). [1][3]
Sources:
[1] https://www.typescriptlang.org/docs/handbook/release-notes/typescript-4-4.html
[2] microsoft/TypeScript#36775
[3] https://www.typescriptlang.org/tsconfig/useUnknownInCatchVariables.html
🏁 Script executed:
# Find and read the useContractWrite.README.md file
find . -name "useContractWrite.README.md" -type f | head -5Repository: TheBlockCade/StellarCade
Length of output: 118
🏁 Script executed:
# Read the file to see the actual content
cat -n ./frontend/src/hooks/v1/useContractWrite.README.md | sed -n '90,150p'Repository: TheBlockCade/StellarCade
Length of output: 2009
Invalid TypeScript syntax in the error handling example.
catch (err: AppError) is not valid TypeScript — catch clause type annotations are restricted to any or unknown only. This will cause a compile error if a reader copies this snippet.
Fix
-} catch (err: AppError) {
+} catch (err: unknown) {
+ const appErr = err as AppError;
console.error(appErr.code);
console.error(appErr.domain);
console.error(appErr.message);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/hooks/v1/useContractWrite.README.md` around lines 120 - 127, The
example uses an invalid TypeScript catch annotation `catch (err: AppError)` for
the `write` call; change the catch to a plain `catch (err)` and then narrow or
cast the error inside the block (e.g., `if (err instanceof AppError) { ... }` or
`const appErr = err as AppError`) before accessing `err.code`, `err.domain`, and
`err.message`, updating the example around the `write('method_name', [param1,
param2])` snippet to reflect the valid pattern.
| // Idempotency check | ||
| if (options?.idempotencyKey) { | ||
| if (idempotencyKeysRef.current.has(options.idempotencyKey)) { | ||
| console.warn( | ||
| `[useContractWrite] Duplicate idempotency key detected: ${options.idempotencyKey}`, | ||
| ); | ||
| } else { | ||
| idempotencyKeysRef.current.add(options.idempotencyKey); | ||
| } | ||
| } | ||
|
|
||
| // Validation: method required | ||
| if (!method || typeof method !== 'string') { | ||
| const error: AppError = { | ||
| code: 'API_VALIDATION_ERROR', | ||
| domain: ErrorDomain.CONTRACT, | ||
| severity: ErrorSeverity.FATAL, | ||
| message: 'Contract method name is required', | ||
| }; | ||
| setState({ phase: 'error', error, confirmations: 0 }); | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
Idempotency key is registered before method validation — key is consumed on invalid calls.
The key is added to idempotencyKeysRef (Line 120) before the !method check (Line 125). If method is empty/invalid, the key is permanently stored, so a subsequent call with the same key (and a now-valid method) incorrectly triggers the duplicate warning even though the first invocation never actually executed.
Fix: move idempotency handling after the method validation.
🐛 Proposed fix
- // Idempotency check
- if (options?.idempotencyKey) {
- if (idempotencyKeysRef.current.has(options.idempotencyKey)) {
- console.warn(
- `[useContractWrite] Duplicate idempotency key detected: ${options.idempotencyKey}`,
- );
- } else {
- idempotencyKeysRef.current.add(options.idempotencyKey);
- }
- }
-
// Validation: method required
if (!method || typeof method !== 'string') {
const error: AppError = {
code: 'API_VALIDATION_ERROR',
domain: ErrorDomain.CONTRACT,
severity: ErrorSeverity.FATAL,
message: 'Contract method name is required',
};
setState({ phase: 'error', error, confirmations: 0 });
throw error;
}
+ // Idempotency check
+ if (options?.idempotencyKey) {
+ if (idempotencyKeysRef.current.has(options.idempotencyKey)) {
+ console.warn(
+ `[useContractWrite] Duplicate idempotency key detected: ${options.idempotencyKey}`,
+ );
+ } else {
+ idempotencyKeysRef.current.add(options.idempotencyKey);
+ }
+ }
+
setState({ phase: 'loading', confirmations: 0 });📝 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.
| // Idempotency check | |
| if (options?.idempotencyKey) { | |
| if (idempotencyKeysRef.current.has(options.idempotencyKey)) { | |
| console.warn( | |
| `[useContractWrite] Duplicate idempotency key detected: ${options.idempotencyKey}`, | |
| ); | |
| } else { | |
| idempotencyKeysRef.current.add(options.idempotencyKey); | |
| } | |
| } | |
| // Validation: method required | |
| if (!method || typeof method !== 'string') { | |
| const error: AppError = { | |
| code: 'API_VALIDATION_ERROR', | |
| domain: ErrorDomain.CONTRACT, | |
| severity: ErrorSeverity.FATAL, | |
| message: 'Contract method name is required', | |
| }; | |
| setState({ phase: 'error', error, confirmations: 0 }); | |
| throw error; | |
| } | |
| // Validation: method required | |
| if (!method || typeof method !== 'string') { | |
| const error: AppError = { | |
| code: 'API_VALIDATION_ERROR', | |
| domain: ErrorDomain.CONTRACT, | |
| severity: ErrorSeverity.FATAL, | |
| message: 'Contract method name is required', | |
| }; | |
| setState({ phase: 'error', error, confirmations: 0 }); | |
| throw error; | |
| } | |
| // Idempotency check | |
| if (options?.idempotencyKey) { | |
| if (idempotencyKeysRef.current.has(options.idempotencyKey)) { | |
| console.warn( | |
| `[useContractWrite] Duplicate idempotency key detected: ${options.idempotencyKey}`, | |
| ); | |
| } else { | |
| idempotencyKeysRef.current.add(options.idempotencyKey); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/hooks/v1/useContractWrite.ts` around lines 113 - 134, The
idempotency key is being consumed before validating the contract method, causing
invalid calls to permanently register keys; move the idempotency handling block
that checks and adds options.idempotencyKey (uses
idempotencyKeysRef.current.has(...) and .add(...)) to after the method
validation (the if (!method || typeof method !== 'string') branch where
setState({ phase: 'error', ... }) and throw error occur) so keys are only
recorded for requests that pass validation; ensure the duplicate warning logic
remains the same but executes after the method check.
| } catch (err: unknown) { | ||
| const mappedError = mapRpcError(err, { | ||
| method, | ||
| params, | ||
| wallet: walletMeta?.publicKey, | ||
| }); |
There was a problem hiding this comment.
Wallet public key logged as PII in error context.
walletMeta?.publicKey is passed as wallet inside the error context forwarded to mapRpcError. A Stellar public key uniquely identifies a user. If mapRpcError forwards this context to a logging/monitoring backend, it constitutes PII retention without explicit consent and can be a GDPR/CCPA compliance risk.
Consider omitting it, hashing it, or truncating to a prefix for debugging purposes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/hooks/v1/useContractWrite.ts` around lines 154 - 159, The error
context currently passes a raw Stellar public key (walletMeta?.publicKey) into
mapRpcError which can leak PII; update the catch block in useContractWrite to
redact before forwarding: create a redacted value (e.g., hashed or
truncated/prefixed such as first 8 chars + ellipsis) and pass that as the wallet
field to mapRpcError (replace walletMeta?.publicKey with the redacted variable),
or omit the wallet field entirely if not needed for debugging; ensure the
redaction logic runs locally in the catch block so only the safe token is sent
to mappedError.
| export async function executeContractMethod<TData>( | ||
| client: SorobanContractClient, | ||
| method: string, | ||
| params: any[] = [], | ||
| options?: ContractWriteOptions, | ||
| ): Promise<{ data: TData; txHash?: string; confirmations?: number }> { | ||
| const executor = CONTRACT_METHOD_REGISTRY[method as ContractMethodName]; | ||
|
|
||
| if (!executor) { | ||
| throw new Error( | ||
| `Contract method '${method}' not found in registry. ` + | ||
| `Available methods: ${Object.keys(CONTRACT_METHOD_REGISTRY).join(', ')}` | ||
| ); | ||
| } | ||
|
|
||
| // Execute the method through the client | ||
| const result = await executor(client, params); | ||
|
|
||
| // SorobanContractClient returns ContractResult<T> | ||
| if (!result.success) { | ||
| throw result.error; | ||
| } | ||
|
|
||
| return { | ||
| data: result.data as TData, | ||
| txHash: result.txHash, | ||
| ledger: result.ledger, | ||
| confirmations: result.ledger ? 1 : 0, | ||
| }; | ||
| } |
There was a problem hiding this comment.
options parameter is accepted but never used — confirmationTimeoutMs and pollIntervalMs are dead features.
The options?: ContractWriteOptions argument is received but never referenced in the function body. The timeout and polling interval documented in the README and typed in ContractWriteOptions have no effect. Callers passing confirmationTimeoutMs: 60000 will silently get the default behaviour with no indication anything is wrong.
Either implement the options or remove the parameter until they are supported, to avoid misleading consumers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/utils/v1/contractMethodRegistry.ts` around lines 53 - 82,
executeContractMethod currently accepts options?: ContractWriteOptions but never
uses confirmationTimeoutMs or pollIntervalMs; either implement polling using
those values or remove the parameter. Fix by adding post-execution confirmation
polling after calling executor(client, params): if result.ledger is undefined
(or confirmations are required), repeatedly call client.getTransaction or the
executor's status-check helper (referencing executeContractMethod,
CONTRACT_METHOD_REGISTRY, and SorobanContractClient) at intervals defined by
options.pollIntervalMs until either confirmations are observed or
options.confirmationTimeoutMs elapses, then return data/txHash/confirmations;
ensure default timeout/interval fallbacks are applied when options is absent.
| ): Promise<{ data: TData; txHash?: string; confirmations?: number }> { | ||
| const executor = CONTRACT_METHOD_REGISTRY[method as ContractMethodName]; | ||
|
|
||
| if (!executor) { | ||
| throw new Error( | ||
| `Contract method '${method}' not found in registry. ` + | ||
| `Available methods: ${Object.keys(CONTRACT_METHOD_REGISTRY).join(', ')}` | ||
| ); | ||
| } | ||
|
|
||
| // Execute the method through the client | ||
| const result = await executor(client, params); | ||
|
|
||
| // SorobanContractClient returns ContractResult<T> | ||
| if (!result.success) { | ||
| throw result.error; | ||
| } | ||
|
|
||
| return { | ||
| data: result.data as TData, | ||
| txHash: result.txHash, | ||
| ledger: result.ledger, | ||
| confirmations: result.ledger ? 1 : 0, | ||
| }; |
There was a problem hiding this comment.
ledger in the return literal is not part of the declared return type — TypeScript excess-property error.
The function signature declares Promise<{ data: TData; txHash?: string; confirmations?: number }>, but the returned object literal at Line 79 includes ledger: result.ledger. TypeScript's excess property checking on return statements in strict mode flags this as an error. Additionally, the hook at useContractWrite.ts Line 150 uses result.confirmations ?? 1, meaning if confirmations comes back as 0 (falsy) it will be silently overridden to 1 — an unintended default.
🐛 Proposed fix
export async function executeContractMethod<TData>(
client: SorobanContractClient,
method: string,
params: any[] = [],
options?: ContractWriteOptions,
-): Promise<{ data: TData; txHash?: string; confirmations?: number }> {
+): Promise<{ data: TData; txHash?: string; ledger?: number; confirmations: number }> {
...
return {
data: result.data as TData,
txHash: result.txHash,
ledger: result.ledger,
- confirmations: result.ledger ? 1 : 0,
+ confirmations: result.confirmations ?? (result.ledger ? 1 : 0),
};
}Then update useContractWrite.ts Line 150 to remove the ?? 1 override:
- confirmations: result.confirmations ?? 1,
+ confirmations: result.confirmations,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/utils/v1/contractMethodRegistry.ts` around lines 58 - 81, The
returned object from the executor includes an extra property "ledger" that isn't
in the declared return type of the function, causing a TypeScript
excess-property error; remove "ledger: result.ledger" from the returned object
(or alternatively expand the Promise return type to include ledger if you intend
to return it) and keep the returned shape consistent with Promise<{ data: TData;
txHash?: string; confirmations?: number }>. Also update the consumer in
useContractWrite (the hook using result.confirmations) to stop using the
falsy-coalescing override "?? 1" which converts 0 to 1; instead use an explicit
undefined check (e.g., confirmations === undefined ? 1 : confirmations) or
simply propagate the confirmations value so that a 0 confirmation remains 0.
Ensure changes reference the executor invocation and the
CONTRACT_METHOD_REGISTRY lookup so the returned shape and the hook's handling
stay aligned.
| it('should reject write on unsupported network', async () => { | ||
| // Override mock for network guard | ||
| (useNetworkGuardModule.useNetworkGuard as Mock).mockReturnValue({ | ||
| ...mockNetworkGuard, | ||
| assertSupportedNetwork: vi.fn(() => ({ | ||
| isSupported: false, | ||
| actual: 'unknown', | ||
| supportedNetworks: ['testnet', 'mainnet'], | ||
| message: 'Network unknown is not supported', | ||
| })), | ||
| }); | ||
|
|
||
| const { result } = renderHook(() => useContractWrite()); | ||
|
|
||
| await act(async () => { | ||
| try { | ||
| await result.current.write('test_method'); | ||
| expect.fail('Should have thrown an error'); | ||
| } catch (error: any) { | ||
| expect(error.code).toBe('WALLET_NETWORK_MISMATCH'); | ||
| } | ||
| }); | ||
|
|
||
| expect(result.current.phase).toBe('error'); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and examine assertSupportedNetworkUtil implementation
rg -n "assertSupportedNetworkUtil|assertSupportedNetwork" --type ts -A 10 | head -100Repository: TheBlockCade/StellarCade
Length of output: 7746
🏁 Script executed:
# Get the test file to review the specific lines mentioned
git ls-files | grep -E "useContractWrite\.test\.ts$"Repository: TheBlockCade/StellarCade
Length of output: 116
🏁 Script executed:
# Look at the test file structure
fd "useContractWrite.test.ts" -type fRepository: TheBlockCade/StellarCade
Length of output: 238
🏁 Script executed:
# Get the full implementation of assertSupportedNetwork
cat -n frontend/src/utils/v1/useNetworkGuard.ts | sed -n '172,195p'Repository: TheBlockCade/StellarCade
Length of output: 735
🏁 Script executed:
# Get the interface/type definition for AssertNetworkResult
rg -n "AssertNetworkResult|interface.*AssertNetworkResult" --type ts -A 8Repository: TheBlockCade/StellarCade
Length of output: 2750
🏁 Script executed:
# Check how the hook uses assertSupportedNetwork
cat -n frontend/src/hooks/v1/useContractWrite.ts | sed -n '90,115p'Repository: TheBlockCade/StellarCade
Length of output: 1147
🏁 Script executed:
# Get the hook definition and see how assertSupportedNetwork is called
cat -n frontend/src/hooks/v1/useNetworkGuard.ts | head -50Repository: TheBlockCade/StellarCade
Length of output: 2042
🏁 Script executed:
# Check the full test file to see how the mock is being used
cat -n frontend/tests/hooks/v1/useContractWrite.test.ts | sed -n '1,60p'Repository: TheBlockCade/StellarCade
Length of output: 2504
🏁 Script executed:
# Check what properties assertSupportedNetwork actually returns
cat -n frontend/src/hooks/v1/useNetworkGuard.ts | sed -n '40,60p'Repository: TheBlockCade/StellarCade
Length of output: 719
🏁 Script executed:
# Check if there's a property mismatch - the code checks 'isSupported' but real function returns 'ok'
rg -n "isSupported|\.ok" frontend/src/hooks/v1/useContractWrite.ts -B 2 -A 2Repository: TheBlockCade/StellarCade
Length of output: 306
🏁 Script executed:
# Check the exact import and usage pattern in useContractWrite
cat -n frontend/src/hooks/v1/useContractWrite.ts | sed -n '1,40p'Repository: TheBlockCade/StellarCade
Length of output: 1691
🏁 Script executed:
# Check the test more carefully - let me see how the network check result is used
cat -n frontend/tests/hooks/v1/useContractWrite.test.ts | sed -n '114,140p'Repository: TheBlockCade/StellarCade
Length of output: 1160
🏁 Script executed:
# Check if the hook's useNetworkGuard has a different return signature
cat -n frontend/src/hooks/v1/useNetworkGuard.ts | sed -n '17,53p'Repository: TheBlockCade/StellarCade
Length of output: 1619
🏁 Script executed:
# Verify if 'ok' vs 'isSupported' property is the real issue
rg -n "\.ok\b|\.isSupported\b" frontend/src/hooks/v1/useContractWrite.ts -B 1 -A 1Repository: TheBlockCade/StellarCade
Length of output: 215
🏁 Script executed:
# Let me verify the exact property name the real function returns
cat -n frontend/src/utils/v1/useNetworkGuard.ts | sed -n '172,191p'Repository: TheBlockCade/StellarCade
Length of output: 735
🏁 Script executed:
# Check line 98 in useContractWrite to confirm the property being checked
cat -n frontend/src/hooks/v1/useContractWrite.ts | sed -n '96,112p'Repository: TheBlockCade/StellarCade
Length of output: 772
🏁 Script executed:
# Check if there's a property alias or if message and actual properties exist
cat -n frontend/src/hooks/v1/useContractWrite.ts | sed -n '96,111p'Repository: TheBlockCade/StellarCade
Length of output: 764
🏁 Script executed:
# Verify if the AssertNetworkResult interface has isSupported or ok
cat -n frontend/src/utils/v1/useNetworkGuard.ts | sed -n '38,43p'Repository: TheBlockCade/StellarCade
Length of output: 264
🏁 Script executed:
# Let me check the actual properties being accessed in the hook code
cat -n frontend/src/hooks/v1/useContractWrite.ts | sed -n '96,112p'Repository: TheBlockCade/StellarCade
Length of output: 772
🏁 Script executed:
# Check the complete interface definition
rg -n "interface AssertNetworkResult" -A 10 frontend/src/utils/v1/useNetworkGuard.tsRepository: TheBlockCade/StellarCade
Length of output: 339
🏁 Script executed:
# Search for any type extensions or overloads
rg -n "isSupported|\.ok" frontend/src/hooks/v1/useNetworkGuard.ts -B 2 -A 2Repository: TheBlockCade/StellarCade
Length of output: 923
🏁 Script executed:
# Check if there's a different definition or if the code is using a different property
rg -n "message.*actual" frontend/src/hooks/v1/useContractWrite.tsRepository: TheBlockCade/StellarCade
Length of output: 50
🏁 Script executed:
# Let me see if NetworkSupportError has these properties
cat -n frontend/src/utils/v1/useNetworkGuard.ts | sed -n '22,37p'Repository: TheBlockCade/StellarCade
Length of output: 606
🏁 Script executed:
# Check if the error object has the properties being accessed
rg -n "interface NetworkSupportError" -A 5 frontend/src/utils/v1/useNetworkGuard.tsRepository: TheBlockCade/StellarCade
Length of output: 318
🏁 Script executed:
# Check if there's a TypeScript error by looking at the actual property names being used
cat -n frontend/src/hooks/v1/useContractWrite.ts | sed -n '96,112p'Repository: TheBlockCade/StellarCade
Length of output: 772
🏁 Script executed:
# Verify the complete AssertNetworkResult definition - check if there's an isSupported property
rg -n "AssertNetworkResult\|ok\|isSupported" frontend/src/utils/v1/useNetworkGuard.ts | head -20Repository: TheBlockCade/StellarCade
Length of output: 50
🏁 Script executed:
# Check if the code is using wrong property names - should be 'ok' not 'isSupported'
cat -n frontend/src/hooks/v1/useContractWrite.ts | sed -n '97,111p'Repository: TheBlockCade/StellarCade
Length of output: 716
🏁 Script executed:
# Let me check if there's a type mismatch - the real function returns 'ok' but code uses 'isSupported'
# Check the full interface and see if there's an accessor or alias
rg -n "type.*AssertNetworkResult|interface.*AssertNetworkResult" -A 15 frontend/src/utils/v1/useNetworkGuard.tsRepository: TheBlockCade/StellarCade
Length of output: 502
🏁 Script executed:
# Check if the code compiles - see if there's a TypeScript error or a different interface being used
cat -n frontend/src/hooks/v1/useContractWrite.ts | head -30Repository: TheBlockCade/StellarCade
Length of output: 1354
🏁 Script executed:
# Check how networkGuard is typed in the hook
rg -n "networkGuard" frontend/src/hooks/v1/useContractWrite.ts -B 2 -A 2 | head -20Repository: TheBlockCade/StellarCade
Length of output: 622
🏁 Script executed:
# Check if there's a TypeScript compilation error or if properties are being added somewhere
rg -n "isSupported|\.message|\.actual" frontend/src/hooks/v1/useContractWrite.ts -B 1 -A 1Repository: TheBlockCade/StellarCade
Length of output: 446
🏁 Script executed:
# Check if there's a computed property or if the AssertNetworkResult is being extended
rg -n "AssertNetworkResult" frontend/src/hooks/v1/useContractWrite.ts -B 5 -A 5Repository: TheBlockCade/StellarCade
Length of output: 50
🏁 Script executed:
# Let me check the error object structure more carefully
cat -n frontend/src/utils/v1/useNetworkGuard.ts | sed -n '172,191p'Repository: TheBlockCade/StellarCade
Length of output: 735
🏁 Script executed:
# Let me check the complete code flow - how the error object is being accessed
cat -n frontend/src/hooks/v1/useContractWrite.ts | sed -n '177,184p'Repository: TheBlockCade/StellarCade
Length of output: 354
🏁 Script executed:
# Check if there's a spread or destructuring of the error object
rg -n "error\?.error|error\?" frontend/src/hooks/v1/useContractWrite.ts -B 2 -A 2Repository: TheBlockCade/StellarCade
Length of output: 164
🏁 Script executed:
# Let me verify - the code uses networkCheck.isSupported but AssertNetworkResult has 'ok'
# Check if there's a computed property or getter
cat -n frontend/src/utils/v1/useNetworkGuard.ts | sed -n '38,60p'Repository: TheBlockCade/StellarCade
Length of output: 754
🏁 Script executed:
# Check if there are properties being spread from error object
cat -n frontend/src/hooks/v1/useContractWrite.ts | sed -n '96,112p'Repository: TheBlockCade/StellarCade
Length of output: 772
🏁 Script executed:
# Let me verify the actual type by checking return path
cat -n frontend/src/utils/v1/useNetworkGuard.ts | sed -n '176,191p'Repository: TheBlockCade/StellarCade
Length of output: 567
🏁 Script executed:
# This is critical - let me verify if the code even compiles
# Check if there's a property accessor or if this is a type error
rg -n "isSupported\|\.ok\b" frontend/src/hooks/v1/useContractWrite.tsRepository: TheBlockCade/StellarCade
Length of output: 50
🏁 Script executed:
# Check if the error object is structured with nested error property
cat -n frontend/src/hooks/v1/useContractWrite.ts | sed -n '96,112p'Repository: TheBlockCade/StellarCade
Length of output: 772
🏁 Script executed:
# Verify the actual return structure when ok is false
cat -n frontend/src/utils/v1/useNetworkGuard.ts | sed -n '177,183p'Repository: TheBlockCade/StellarCade
Length of output: 313
🏁 Script executed:
# I see a critical issue - let me verify the exact structure
# When ok is false, the return has an 'error' property, not message/actual at top level
# Check if there's property spreading
cat -n frontend/src/utils/v1/useNetworkGuard.ts | sed -n '176,191p'Repository: TheBlockCade/StellarCade
Length of output: 567
🏁 Script executed:
# Verify what properties are actually on the return object when unsupported
python3 << 'EOF'
# Parse the function to understand the exact return structure
import re
return_fail = """
return {
ok: false,
normalizedActual: support.normalizedActual,
supportedNetworks: support.supportedNetworks,
error: support.error,
};
"""
return_success = """
return {
ok: true,
normalizedActual: support.normalizedActual,
supportedNetworks: support.supportedNetworks,
};
"""
print("When unsupported (ok: false):")
print("- ok: false")
print("- normalizedActual: string")
print("- supportedNetworks: readonly string[]")
print("- error: NetworkSupportError")
print()
print("When supported (ok: true):")
print("- ok: true")
print("- normalizedActual: string")
print("- supportedNetworks: readonly string[]")
print()
print("Hook code accesses:")
print("- networkCheck.isSupported (WRONG - should be .ok)")
print("- networkCheck.message (WRONG - should be .error?.message)")
print("- networkCheck.actual (WRONG - should be .error?.actual)")
EOFRepository: TheBlockCade/StellarCade
Length of output: 497
🏁 Script executed:
# Check if AssertNetworkResult has been modified to include spreading
rg -n "interface AssertNetworkResult|type AssertNetworkResult" -A 20 frontend/src/utils/v1/useNetworkGuard.tsRepository: TheBlockCade/StellarCade
Length of output: 617
🏁 Script executed:
# This is critical - check if the code even compiles with these property mismatches
# Let me verify if there's a TypeScript configuration that allows this
cat -n frontend/tsconfig.json | head -30Repository: TheBlockCade/StellarCade
Length of output: 948
🏁 Script executed:
# Check if maybe there's a type casting or property spreading that reconciles this
cat -n frontend/src/hooks/v1/useContractWrite.ts | sed -n '85,115p'Repository: TheBlockCade/StellarCade
Length of output: 1386
🏁 Script executed:
# Verify if the production code has type errors
rg -n "tsconfig\|suppress" frontend/ -l | head -5Repository: TheBlockCade/StellarCade
Length of output: 50
Critical type mismatch: Production code accesses non-existent properties on AssertNetworkResult.
The hook code at line 98 checks networkCheck.isSupported, but the real assertSupportedNetwork function returns an object with the property ok, not isSupported. Additionally, lines 103–105 access networkCheck.message and networkCheck.actual, but these properties are nested within the optional error object in the actual return type.
The correct check should be:
- if (!networkCheck.isSupported) {
+ if (!networkCheck.ok) {
const error: AppError = {
code: 'WALLET_NETWORK_MISMATCH',
domain: ErrorDomain.WALLET,
severity: ErrorSeverity.USER_ACTIONABLE,
- message: networkCheck.message,
+ message: networkCheck.error?.message || 'Network is not supported',
context: {
- actual: networkCheck.actual,
+ actual: networkCheck.error?.actual,
expected: networkCheck.supportedNetworks,
},
};The test mock at lines 118–123 currently mirrors the hook's incorrect assumptions (returning isSupported, message, actual at the top level), which masks this type error. Update the mock to match the actual AssertNetworkResult interface structure once the hook is corrected.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/tests/hooks/v1/useContractWrite.test.ts` around lines 114 - 138, The
hook useContractWrite incorrectly assumes assertSupportedNetwork returns
top-level fields like isSupported, message, and actual; update the hook to treat
the return as an AssertNetworkResult (check result.ok rather than isSupported
and read error?.message and error?.actual from the nested error object), adjust
any logic that sets phase or throws WALLET_NETWORK_MISMATCH accordingly, and
then update the test mock (useNetworkGuardModule.useNetworkGuard) to return the
real AssertNetworkResult shape (e.g., { ok: false, error: { message: '...',
actual: '...', supportedNetworks: [...] } }) so the test no longer masks the
type mismatch.
| act(() => { | ||
| result.current.reset(); | ||
| }); |
There was a problem hiding this comment.
act() should be awaited even for synchronous state updates in RTL v16.
act() in @testing-library/react v16 returns a Promise; not awaiting it can suppress React act-warning flushes in some environments.
♻️ Proposed fix
- act(() => {
+ await act(async () => {
result.current.reset();
});📝 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.
| act(() => { | |
| result.current.reset(); | |
| }); | |
| await act(async () => { | |
| result.current.reset(); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/tests/hooks/v1/useContractWrite.test.ts` around lines 248 - 250, The
test calls act(() => { result.current.reset(); }) synchronously; change the test
to await the act() call (e.g., make the test async and use await act(() =>
result.current.reset())) so React's async act from RTL v16 is properly awaited;
update the test declaration to async and replace the act invocation referencing
act and result.current.reset accordingly.
|
@soma-enyi resolve the conflicts |
|
why cant i link my solution on drips? |
|
@ CMI_james pls merge my PR |
3 participants
Closes #112
Closes #[issue-number]
Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context.
Fixes # (issue)
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes.
Checklist:
Summary by CodeRabbit
New Features
Documentation
Tests