feat: add gasToken option to execute and deploy#119
feat: add gasToken option to execute and deploy#1190xLucqs merged 9 commits intokeep-starknet-strange:mainfrom
Conversation
Add optional `gasToken` field to `ExecuteOptions` and `DeployOptions`
so users can pay gas fees with ERC-20 tokens (USDC, STRK, USDT, etc.)
through the AVNU Paymaster without monkey-patching.
When `gasToken` is provided, the SDK uses `executePaymasterTransaction`
with `{ mode: 'default', gasToken }` instead of `{ mode: 'sponsored' }`.
Changes:
- `src/types/wallet.ts`: Add `gasToken?: Address` to ExecuteOptions and
DeployOptions
- `src/wallet/utils.ts`: Extend `sponsoredDetails()` to accept gasToken
and produce the correct PaymasterFeeMode
- `src/wallet/index.ts`: Thread gasToken through execute, deploy, and
Braavos factory flows
- `src/wallet/cartridge.ts`: Same for CartridgeWallet
…et.deploy() Two issues found during audit: 1. Wallet.deploy() was not reading gasToken from DeployOptions, silently ignoring it. Now reads gasToken and routes to the paymaster path when set (same pattern as execute()). 2. CartridgeWallet.deploy() now guards against gasToken in addition to feeMode/timeBounds, since Cartridge Controller manages its own deployment and does not support external paymaster overrides.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds optional gasToken?: Address to wallet options and centralizes fee fields into TransactionFeeOptions. Wallet deploy/execute flows treat presence of gasToken as a paymaster config, propagate it through paymaster paths, and validate it cannot be combined with feeMode: "user_pays". Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/App
participant Wallet as Wallet
participant Utils as paymasterDetails
participant Account as Account/Paymaster API
User->>Wallet: execute(calls, { gasToken: USDC })
Wallet->>Wallet: determine paymaster path (feeMode==="sponsored" OR gasToken)
Wallet->>Utils: paymasterDetails({ timeBounds?, deploymentData?, gasToken })
Utils->>Utils: compute feeMode = { mode: "default", gasToken }
Utils-->>Wallet: return paymaster details
Wallet->>Account: executePaymasterTransaction(calls, { feeMode, timeBounds })
Account-->>User: Transaction executed (gas token used)
sequenceDiagram
participant User as User/App
participant Wallet as Wallet
participant Utils as paymasterDetails
participant Account as Paymaster API / Factory
User->>Wallet: deploy(deploymentData, { gasToken: USDC })
Wallet->>Wallet: determine paymaster path (feeMode==="sponsored" OR gasToken)
Wallet->>Utils: paymasterDetails({ timeBounds?, deploymentData, gasToken })
Utils->>Utils: compute feeMode = { mode: "default", gasToken }
Utils-->>Wallet: return paymaster details
Wallet->>Account: deploy via paymaster/factory with { feeMode }
Account-->>User: Contract deployed (gas token used)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 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)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/wallet/index.ts (1)
355-384: 🧹 Nitpick | 🔵 TrivialUse
Addresstype instead ofstringfor internalgasTokenparameters.The internal methods (
deployPaymasterWith,deployBraavosViaFactory,executePaymaster,executeSponsored) usestringfor thegasTokenparameter, but the public API types it asAddress. For consistency and type safety, useAddressthroughout.♻️ Proposed signature changes
+import type { Address } from "@/types"; private async deployPaymasterWith( calls: Call[], timeBounds?: PaymasterTimeBounds, - gasToken?: string + gasToken?: Address ): Promise<Tx> {Apply similar changes to
deployBraavosViaFactory,executePaymaster, andexecuteSponsored.As per coding guidelines: "Preserve strict TypeScript typing; prefer explicit domain types (Address, Amount, ChainId) over primitives".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/wallet/index.ts` around lines 355 - 384, The internal functions use a primitive string for gasToken while the public API uses Address; update the signatures and usages to use the Address type instead of string for gasToken across deployPaymasterWith, deployBraavosViaFactory, executePaymaster, and executeSponsored (and any other internal helpers), update local variables and calls (e.g., the gasToken parameter passed into sponsoredDetails and executePaymaster) to the Address type, and ensure any imports/exports and callers are updated to match the new type to preserve strict typing and avoid implicit string usage.src/wallet/utils.ts (1)
147-162: 🛠️ Refactor suggestion | 🟠 MajorUse
Addresstype instead ofstringforgasTokenparameter.The
gasTokenparameter is typed asstring, but should be typed asAddressto maintain consistency with type definitions insrc/types/wallet.tswhereDeployOptionsandExecuteOptionsdefinegasToken?: Address. This preserves strict TypeScript typing throughout the SDK.♻️ Proposed fix
export function sponsoredDetails( timeBounds?: PaymasterTimeBounds, deploymentData?: PAYMASTER_API.ACCOUNT_DEPLOYMENT_DATA, - gasToken?: string + gasToken?: Address ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/wallet/utils.ts` around lines 147 - 162, Change the gasToken parameter in the sponsoredDetails function to use the Address type instead of string: import the Address type from the wallet/types module (or where Address is defined) and update the function signature sponsoredDetails(..., gasToken?: Address) so callers and returned feeMode.gasToken remain strongly typed; keep the existing conditional logic that chooses between { mode: "default", gasToken } and { mode: "sponsored" } and update any related JSDoc or type annotations if present (function name: sponsoredDetails; referenced types: Address, PaymasterTimeBounds, PAYMASTER_API.ACCOUNT_DEPLOYMENT_DATA).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/wallet/index.ts`:
- Around line 290-293: The code currently treats a provided gasToken as taking
precedence over feeMode, silently routing to deployPaymasterWith; add a guard to
detect the conflicting options (when feeMode === "user_pays" and gasToken is
truthy) and throw a clear, descriptive Error (or TypeError) indicating the
conflict and instructing the caller to choose one option; update the validation
near where gasToken and feeMode are read (references: gasToken, feeMode,
deployPaymasterWith, ExecuteOptions/DeployOptions) so callers get immediate
feedback instead of implicit precedence.
---
Outside diff comments:
In `@src/wallet/index.ts`:
- Around line 355-384: The internal functions use a primitive string for
gasToken while the public API uses Address; update the signatures and usages to
use the Address type instead of string for gasToken across deployPaymasterWith,
deployBraavosViaFactory, executePaymaster, and executeSponsored (and any other
internal helpers), update local variables and calls (e.g., the gasToken
parameter passed into sponsoredDetails and executePaymaster) to the Address
type, and ensure any imports/exports and callers are updated to match the new
type to preserve strict typing and avoid implicit string usage.
In `@src/wallet/utils.ts`:
- Around line 147-162: Change the gasToken parameter in the sponsoredDetails
function to use the Address type instead of string: import the Address type from
the wallet/types module (or where Address is defined) and update the function
signature sponsoredDetails(..., gasToken?: Address) so callers and returned
feeMode.gasToken remain strongly typed; keep the existing conditional logic that
chooses between { mode: "default", gasToken } and { mode: "sponsored" } and
update any related JSDoc or type annotations if present (function name:
sponsoredDetails; referenced types: Address, PaymasterTimeBounds,
PAYMASTER_API.ACCOUNT_DEPLOYMENT_DATA).
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ae3e1d7e-32cf-4c09-a93d-71b190098bf9
📒 Files selected for processing (4)
src/types/wallet.tssrc/wallet/cartridge.tssrc/wallet/index.tssrc/wallet/utils.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use@/path alias for internal imports in TypeScript source files
Use strict TypeScript compiler settings:strict,noUncheckedIndexedAccess,exactOptionalPropertyTypesenabled
Files:
src/wallet/cartridge.tssrc/wallet/utils.tssrc/types/wallet.tssrc/wallet/index.ts
src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Preserve strict TypeScript typing; prefer explicit domain types (Address, Amount, ChainId) over primitives
Files:
src/wallet/cartridge.tssrc/wallet/utils.tssrc/types/wallet.tssrc/wallet/index.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not introduce CommonJS patterns; use ESM only
Files:
src/wallet/cartridge.tssrc/wallet/utils.tssrc/types/wallet.tssrc/wallet/index.ts
{src/sdk.ts,src/wallet/**,src/types/**}
📄 CodeRabbit inference engine (AGENTS.md)
Must serialize: Concurrent edits in
src/sdk.ts,src/wallet/*, or sharedsrc/types/*
Files:
src/wallet/cartridge.tssrc/wallet/utils.tssrc/types/wallet.tssrc/wallet/index.ts
src/**
⚙️ CodeRabbit configuration file
src/**: This is a public SDK. Prioritize:
- Backwards compatibility and clear, typed public APIs
- Deterministic behavior across mainnet/sepolia presets
- Tests for behavior changes (prefer unit tests)
Files:
src/wallet/cartridge.tssrc/wallet/utils.tssrc/types/wallet.tssrc/wallet/index.ts
🧠 Learnings (2)
📚 Learning: 2026-03-17T08:43:15.273Z
Learnt from: ZackAttax
Repo: keep-starknet-strange/starkzap PR: 86
File: packages/native/src/cartridge/ts/outside_execution_v3.ts:36-38
Timestamp: 2026-03-17T08:43:15.273Z
Learning: In `packages/native/src/cartridge/ts/outside_execution_v3.ts`, the `SESSION_TYPE_HASH` encodes only 4 fields ("Expires At", "Allowed Methods", "Metadata", "Session Key") while `hashSessionStruct` serializes 5 fields (adding `guardianKeyGuid`). This mismatch is intentional and inherited from the upstream Cartridge protocol (controller-rs / controller.c session token shape), where the same 4-field type-hash / 5-field payload pattern exists. Changing starkzap alone would break protocol parity and invalidate outside-execution signatures against live Cartridge contracts. This should only be fixed upstream as a coordinated/versioned protocol change.
Applied to files:
src/wallet/cartridge.ts
📚 Learning: 2026-03-09T13:39:32.603Z
Learnt from: adrienlacombe
Repo: keep-starknet-strange/starkzap PR: 62
File: src/tx/builder.ts:449-539
Timestamp: 2026-03-09T13:39:32.603Z
Learning: Guideline for confidential transaction handling in StarkZap: In src/tx/builder.ts and src/confidential/types.ts, the sender field for confidential/tongo operations (ConfidentialFundDetails, ConfidentialTransferDetails, ConfidentialWithdrawDetails, ConfidentialRagequitDetails, ConfidentialRolloverDetails) represents the Starknet account submitting the transaction (often a relayer), not necessarily the wallet owner. The optional feeTo field is for relayed transactions where sender differs from the wallet.
Actionable checks for code reviews:
- Do not override details.sender with this.wallet.address in TxBuilder confidential methods; doing so breaks the relayer pattern.
- Ensure staking methods continue to bind this.wallet.address to reflect the pool member’s own address (no relayer concept there).
- If using a relayer, verify feeTo is set when appropriate and that sender remains the relayer address.
- Update or add tests to reflect the correct sender semantics for confidential operations...
Applied to files:
src/wallet/cartridge.tssrc/wallet/utils.tssrc/types/wallet.tssrc/wallet/index.ts
🔇 Additional comments (5)
src/wallet/cartridge.ts (2)
288-296: LGTM!The guard correctly rejects
gasTokenoverrides forCartridgeWallet.deploy(), which is appropriate since Cartridge Controller manages its own deployment flow. The error message clearly explains the limitation.
313-327: LGTM!The
execute()method correctly:
- Extracts
gasTokenfrom options- Routes to the paymaster path when either
feeMode === "sponsored"orgasTokenis provided- Passes
gasTokentosponsoredDetails()for proper fee mode configurationThis aligns with the PR objective of enabling ERC-20 gas payment via the paymaster.
src/types/wallet.ts (1)
179-188: LGTM!The
gasToken?: Addressadditions to bothDeployOptionsandExecuteOptionsare well-documented with clear JSDoc explaining:
- The purpose (ERC-20 gas payment via paymaster)
- The resulting behavior (
{ mode: 'default', gasToken }instead of full sponsorship)- The prerequisite (paymaster configuration required)
The use of
Addresstype maintains type consistency, and the optional field ensures backwards compatibility.Also applies to: 199-208
src/wallet/index.ts (2)
493-501: LGTM!The
execute()method correctly extractsgasTokenfrom options and routes to the sponsored execution path when eitherfeeMode === "sponsored"orgasTokenis provided, enabling ERC-20 gas payment.
521-556: LGTM!The
executePaymaster()andexecuteSponsored()methods correctly propagate thegasTokenparameter through the entire call chain:
executeSponsored→executePaymaster→sponsoredDetails→ paymaster transaction- Handles both deployed and undeployed account scenarios correctly
- Race condition handling with
withSponsoredDeployLockproperly includesgasToken
Address CodeRabbit review feedback: 1. Use Address type instead of string for all internal gasToken parameters (deployPaymasterWith, deployBraavosViaFactory, executePaymaster, executeSponsored, sponsoredDetails) to maintain consistency with the public API types. 2. Add explicit guard that throws when feeMode 'user_pays' is combined with gasToken in both execute() and deploy(), preventing silent precedence behavior.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/wallet/index.ts (1)
286-297:⚠️ Potential issue | 🔴 CriticalUse the caller-supplied
feeModefor this conflict check.
options.feeModeis normalized againstthis.defaultFeeModebefore the guard. Since wallets default to"user_pays", omittingoptions.feeModestill trips the error whenevergasTokenis set, so the new token-paid path fails on the default configuration and contradicts the "omitfeeMode" guidance.🐛 Proposed fix
async deploy(options: DeployOptions = {}): Promise<Tx> { this.clearDeploymentCache(); - const feeMode = options.feeMode ?? this.defaultFeeMode; + const requestedFeeMode = options.feeMode; + const feeMode = requestedFeeMode ?? this.defaultFeeMode; const timeBounds = options.timeBounds ?? this.defaultTimeBounds; const gasToken = options.gasToken; - if (feeMode === "user_pays" && gasToken) { + if (requestedFeeMode === "user_pays" && gasToken) { throw new Error( "Cannot combine feeMode 'user_pays' with gasToken. " + "Use feeMode 'sponsored' or omit feeMode when paying with an ERC-20 token." ); } @@ async execute(calls: Call[], options: ExecuteOptions = {}): Promise<Tx> { - const feeMode = options.feeMode ?? this.defaultFeeMode; + const requestedFeeMode = options.feeMode; + const feeMode = requestedFeeMode ?? this.defaultFeeMode; const timeBounds = options.timeBounds ?? this.defaultTimeBounds; const gasToken = options.gasToken; - if (feeMode === "user_pays" && gasToken) { + if (requestedFeeMode === "user_pays" && gasToken) { throw new Error( "Cannot combine feeMode 'user_pays' with gasToken. " + "Use feeMode 'sponsored' or omit feeMode when paying with an ERC-20 token." ); }A regression test that leaves the wallet default at
user_paysand then callsdeploy({ gasToken })/execute(..., { gasToken })withoutoptions.feeModewould catch this.As per coding guidelines "This is a public SDK. Prioritize: - Backwards compatibility and clear, typed public APIs - Tests for behavior changes (prefer unit tests)".
Also applies to: 500-510
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/wallet/index.ts` around lines 286 - 297, The guard currently checks the normalized feeMode (const feeMode = options.feeMode ?? this.defaultFeeMode) which treats omitted options.feeMode as "user_pays" and incorrectly throws when gasToken is provided; change the check to only reject the combination when the caller explicitly supplied feeMode === "user_pays" (e.g., use options.feeMode === "user_pays" or explicitly check options.feeMode !== undefined && options.feeMode === "user_pays") before throwing; apply the same fix in the analogous execute method (the other occurrence referenced around lines ~500-510), and add a regression unit test that keeps the wallet default "user_pays" and calls deploy({ gasToken }) / execute(..., { gasToken }) without options.feeMode to ensure no throw.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/wallet/index.ts`:
- Around line 286-297: The guard currently checks the normalized feeMode (const
feeMode = options.feeMode ?? this.defaultFeeMode) which treats omitted
options.feeMode as "user_pays" and incorrectly throws when gasToken is provided;
change the check to only reject the combination when the caller explicitly
supplied feeMode === "user_pays" (e.g., use options.feeMode === "user_pays" or
explicitly check options.feeMode !== undefined && options.feeMode ===
"user_pays") before throwing; apply the same fix in the analogous execute method
(the other occurrence referenced around lines ~500-510), and add a regression
unit test that keeps the wallet default "user_pays" and calls deploy({ gasToken
}) / execute(..., { gasToken }) without options.feeMode to ensure no throw.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 30a12d83-778d-425c-8459-21545adc2651
📒 Files selected for processing (2)
src/wallet/index.tssrc/wallet/utils.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use@/path alias for internal imports in TypeScript source files
Use strict TypeScript compiler settings:strict,noUncheckedIndexedAccess,exactOptionalPropertyTypesenabled
Files:
src/wallet/utils.tssrc/wallet/index.ts
src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Preserve strict TypeScript typing; prefer explicit domain types (Address, Amount, ChainId) over primitives
Files:
src/wallet/utils.tssrc/wallet/index.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not introduce CommonJS patterns; use ESM only
Files:
src/wallet/utils.tssrc/wallet/index.ts
{src/sdk.ts,src/wallet/**,src/types/**}
📄 CodeRabbit inference engine (AGENTS.md)
Must serialize: Concurrent edits in
src/sdk.ts,src/wallet/*, or sharedsrc/types/*
Files:
src/wallet/utils.tssrc/wallet/index.ts
src/**
⚙️ CodeRabbit configuration file
src/**: This is a public SDK. Prioritize:
- Backwards compatibility and clear, typed public APIs
- Deterministic behavior across mainnet/sepolia presets
- Tests for behavior changes (prefer unit tests)
Files:
src/wallet/utils.tssrc/wallet/index.ts
🧠 Learnings (2)
📚 Learning: 2026-03-09T13:39:32.603Z
Learnt from: adrienlacombe
Repo: keep-starknet-strange/starkzap PR: 62
File: src/tx/builder.ts:449-539
Timestamp: 2026-03-09T13:39:32.603Z
Learning: Guideline for confidential transaction handling in StarkZap: In src/tx/builder.ts and src/confidential/types.ts, the sender field for confidential/tongo operations (ConfidentialFundDetails, ConfidentialTransferDetails, ConfidentialWithdrawDetails, ConfidentialRagequitDetails, ConfidentialRolloverDetails) represents the Starknet account submitting the transaction (often a relayer), not necessarily the wallet owner. The optional feeTo field is for relayed transactions where sender differs from the wallet.
Actionable checks for code reviews:
- Do not override details.sender with this.wallet.address in TxBuilder confidential methods; doing so breaks the relayer pattern.
- Ensure staking methods continue to bind this.wallet.address to reflect the pool member’s own address (no relayer concept there).
- If using a relayer, verify feeTo is set when appropriate and that sender remains the relayer address.
- Update or add tests to reflect the correct sender semantics for confidential operations...
Applied to files:
src/wallet/utils.tssrc/wallet/index.ts
📚 Learning: 2026-03-09T13:38:38.248Z
Learnt from: adrienlacombe
Repo: keep-starknet-strange/starkzap PR: 62
File: src/confidential/confidential.ts:104-145
Timestamp: 2026-03-09T13:38:38.248Z
Learning: In `src/confidential/confidential.ts` (and by extension the broader StarkZap SDK), the convention is that `Amount.toBase()` is trusted inside `populate*` methods — no token/decimal metadata validation is performed within the method. This is consistent with ERC20 and Staking modules. `feeTo` in `ConfidentialFundDetails`, `ConfidentialTransferDetails`, etc. is a raw `bigint`, not an `Amount`, and goes straight to the tongo-sdk without conversion.
Applied to files:
src/wallet/utils.tssrc/wallet/index.ts
🔇 Additional comments (1)
src/wallet/utils.ts (1)
147-162: Nice centralization of the paymaster fee-mode translation.Keeping the
{ mode: "default", gasToken }branching in one helper makes the execute/deploy call chain much easier to audit.
The guard was checking the normalized feeMode (which includes the
wallet default 'user_pays'), so execute({ gasToken }) would throw
even though the caller never specified feeMode. Now only throws
when the caller explicitly passes feeMode: 'user_pays' alongside
gasToken.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/wallet/index.ts`:
- Around line 502-516: Extract the duplicated guard into a shared helper named
validateGasTokenCompatibility that accepts (requestedFeeMode, gasToken) and
throws the same Error when requestedFeeMode === "user_pays" && gasToken; then
replace the inline checks in both execute() (around the block using
requestedFeeMode, feeMode, gasToken) and deploy() (the identical guard at lines
~293-298) with calls to validateGasTokenCompatibility(requestedFeeMode,
gasToken) so both paths use the single implementation; ensure the helper is
exported/available where both methods live and keep the original error message
text unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c9b7f65a-b44d-418c-a7c7-0152d7672a66
📒 Files selected for processing (1)
src/wallet/index.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use@/path alias for internal imports in TypeScript source files
Use strict TypeScript compiler settings:strict,noUncheckedIndexedAccess,exactOptionalPropertyTypesenabled
Files:
src/wallet/index.ts
src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Preserve strict TypeScript typing; prefer explicit domain types (Address, Amount, ChainId) over primitives
Files:
src/wallet/index.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not introduce CommonJS patterns; use ESM only
Files:
src/wallet/index.ts
{src/sdk.ts,src/wallet/**,src/types/**}
📄 CodeRabbit inference engine (AGENTS.md)
Must serialize: Concurrent edits in
src/sdk.ts,src/wallet/*, or sharedsrc/types/*
Files:
src/wallet/index.ts
src/**
⚙️ CodeRabbit configuration file
src/**: This is a public SDK. Prioritize:
- Backwards compatibility and clear, typed public APIs
- Deterministic behavior across mainnet/sepolia presets
- Tests for behavior changes (prefer unit tests)
Files:
src/wallet/index.ts
🧠 Learnings (2)
📚 Learning: 2026-03-09T13:39:32.603Z
Learnt from: adrienlacombe
Repo: keep-starknet-strange/starkzap PR: 62
File: src/tx/builder.ts:449-539
Timestamp: 2026-03-09T13:39:32.603Z
Learning: Guideline for confidential transaction handling in StarkZap: In src/tx/builder.ts and src/confidential/types.ts, the sender field for confidential/tongo operations (ConfidentialFundDetails, ConfidentialTransferDetails, ConfidentialWithdrawDetails, ConfidentialRagequitDetails, ConfidentialRolloverDetails) represents the Starknet account submitting the transaction (often a relayer), not necessarily the wallet owner. The optional feeTo field is for relayed transactions where sender differs from the wallet.
Actionable checks for code reviews:
- Do not override details.sender with this.wallet.address in TxBuilder confidential methods; doing so breaks the relayer pattern.
- Ensure staking methods continue to bind this.wallet.address to reflect the pool member’s own address (no relayer concept there).
- If using a relayer, verify feeTo is set when appropriate and that sender remains the relayer address.
- Update or add tests to reflect the correct sender semantics for confidential operations...
Applied to files:
src/wallet/index.ts
📚 Learning: 2026-03-09T13:38:38.248Z
Learnt from: adrienlacombe
Repo: keep-starknet-strange/starkzap PR: 62
File: src/confidential/confidential.ts:104-145
Timestamp: 2026-03-09T13:38:38.248Z
Learning: In `src/confidential/confidential.ts` (and by extension the broader StarkZap SDK), the convention is that `Amount.toBase()` is trusted inside `populate*` methods — no token/decimal metadata validation is performed within the method. This is consistent with ERC20 and Staking modules. `feeTo` in `ConfidentialFundDetails`, `ConfidentialTransferDetails`, etc. is a raw `bigint`, not an `Amount`, and goes straight to the tongo-sdk without conversion.
Applied to files:
src/wallet/index.ts
🔇 Additional comments (4)
src/wallet/index.ts (4)
288-301: LGTM! Guard correctly distinguishes explicit vs defaulted feeMode.The
requestedFeeModepattern ensures the guard only fires when the caller explicitly passesfeeMode: 'user_pays'withgasToken. WhenfeeModeis omitted,requestedFeeModeisundefined, allowinggasTokento route to the paymaster path without error. This properly addresses the previous review feedback.
363-391: LGTM! gasToken propagation through deployment paths is consistent.Both the standard deployment and Braavos factory paths correctly receive and forward
gasTokentosponsoredDetails(). The parameter order matches thesponsoredDetails(timeBounds?, deploymentData?, gasToken?)signature.
537-548: LGTM!The
executePaymastermethod correctly passesundefinedfordeploymentData(since no deployment is occurring) and forwardsgasTokentosponsoredDetails().
550-572: LGTM! All execution branches correctly propagate gasToken.The method handles four distinct scenarios (already deployed, lock re-check, deploy-with-calls, and fallback after concurrent deployment), and all paths consistently forward
gasToken.
DRY up the identical feeMode/gasToken validation in deploy() and execute() into a private static throwGasTokenConflict() helper.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/wallet/index.ts`:
- Around line 399-402: The Braavos deployment path is incorrectly accepting and
passing a gasToken into sponsoredDetails causing the paymaster to charge the
temporary OZ bootstrap account; update deployBraavosViaFactory to reject/ignore
any gasToken (remove the parameter or do not forward it) and ensure the call to
sponsoredDetails uses ozDeploymentData without a gasToken so charges go to the
final Braavos account; also update related callers (deployPaymasterWith and the
public entry points that route to deployBraavosViaFactory) to stop passing
gasToken when initiating the Braavos deployment so the incorrect billing path is
eliminated.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3772daa1-a01e-4ae2-a32f-6d3ac542f2e8
📒 Files selected for processing (1)
src/wallet/index.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use@/path alias for internal imports in TypeScript source files
Use strict TypeScript compiler settings:strict,noUncheckedIndexedAccess,exactOptionalPropertyTypesenabled
Files:
src/wallet/index.ts
src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Preserve strict TypeScript typing; prefer explicit domain types (Address, Amount, ChainId) over primitives
Files:
src/wallet/index.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not introduce CommonJS patterns; use ESM only
Files:
src/wallet/index.ts
{src/sdk.ts,src/wallet/**,src/types/**}
📄 CodeRabbit inference engine (AGENTS.md)
Must serialize: Concurrent edits in
src/sdk.ts,src/wallet/*, or sharedsrc/types/*
Files:
src/wallet/index.ts
src/**
⚙️ CodeRabbit configuration file
src/**: This is a public SDK. Prioritize:
- Backwards compatibility and clear, typed public APIs
- Deterministic behavior across mainnet/sepolia presets
- Tests for behavior changes (prefer unit tests)
Files:
src/wallet/index.ts
🧠 Learnings (2)
📚 Learning: 2026-03-09T13:39:32.603Z
Learnt from: adrienlacombe
Repo: keep-starknet-strange/starkzap PR: 62
File: src/tx/builder.ts:449-539
Timestamp: 2026-03-09T13:39:32.603Z
Learning: Guideline for confidential transaction handling in StarkZap: In src/tx/builder.ts and src/confidential/types.ts, the sender field for confidential/tongo operations (ConfidentialFundDetails, ConfidentialTransferDetails, ConfidentialWithdrawDetails, ConfidentialRagequitDetails, ConfidentialRolloverDetails) represents the Starknet account submitting the transaction (often a relayer), not necessarily the wallet owner. The optional feeTo field is for relayed transactions where sender differs from the wallet.
Actionable checks for code reviews:
- Do not override details.sender with this.wallet.address in TxBuilder confidential methods; doing so breaks the relayer pattern.
- Ensure staking methods continue to bind this.wallet.address to reflect the pool member’s own address (no relayer concept there).
- If using a relayer, verify feeTo is set when appropriate and that sender remains the relayer address.
- Update or add tests to reflect the correct sender semantics for confidential operations...
Applied to files:
src/wallet/index.ts
📚 Learning: 2026-03-09T13:38:38.248Z
Learnt from: adrienlacombe
Repo: keep-starknet-strange/starkzap PR: 62
File: src/confidential/confidential.ts:104-145
Timestamp: 2026-03-09T13:38:38.248Z
Learning: In `src/confidential/confidential.ts` (and by extension the broader StarkZap SDK), the convention is that `Amount.toBase()` is trusted inside `populate*` methods — no token/decimal metadata validation is performed within the method. This is consistent with ERC20 and Staking modules. `feeTo` in `ConfidentialFundDetails`, `ConfidentialTransferDetails`, etc. is a raw `bigint`, not an `Amount`, and goes straight to the tongo-sdk without conversion.
Applied to files:
src/wallet/index.ts
🔇 Additional comments (3)
src/wallet/index.ts (3)
288-299: Good split between requested and effective fee mode.Validating against
options.feeModewhile routing on the defaultedfeeModepreserveswallet.deploy()/execute({ gasToken })even when the wallet default is"user_pays".Also applies to: 499-510
377-381:gasTokennow reaches the actual paymaster payload on the normal path.Passing it into
sponsoredDetails()here avoids silently falling back to sponsored mode for the standard deploy/execute paymaster calls.Also applies to: 537-540
572-576: Nice extraction of the shared conflict guard.Keeping the error text in one place reduces the chance that
deploy()andexecute()drift on future option changes.
|
Hi! Just a gentle ping on this PR in case it got buried. @0xLucqs @micbakos @0xsisyfos would appreciate a quick review when you have time. Thanks! |
0xLucqs
left a comment
0xLucqs
left a comment
There was a problem hiding this comment.
Sorry for the delay on this review! The feature design is solid — threading gasToken through the paymaster path via sponsoredDetails is the right approach. Here are the issues I found, ordered by severity.
Bug
CartridgeWallet.execute() silently ignores explicit feeMode: "user_pays" when gasToken is set
Wallet.execute() correctly guards against { feeMode: "user_pays", gasToken: "0x..." } via throwGasTokenConflict, but CartridgeWallet.execute() has no such check. Because the routing condition is feeMode === "sponsored" || gasToken, a truthy gasToken overrides the explicit "user_pays" and silently routes to the paymaster:
// CartridgeWallet.execute() — current PR code
const feeMode = options.feeMode ?? this.defaultFeeMode; // "user_pays"
const gasToken = options.gasToken; // "0x..."
if (feeMode === "sponsored" || gasToken) { ... } // true — silent overrideFix — mirror Wallet's validation pattern:
const requestedFeeMode = options.feeMode;
const feeMode = requestedFeeMode ?? this.defaultFeeMode;
const gasToken = options.gasToken;
if (requestedFeeMode === "user_pays" && gasToken) {
throw new Error(
"Cannot combine feeMode 'user_pays' with gasToken. " +
"Use feeMode 'sponsored' or omit feeMode when paying with an ERC-20 token."
);
}Consider extracting this into a shared utility in utils.ts so both wallet implementations stay in sync.
Tests
The PR adds zero test coverage. At minimum these scenarios should be covered before merge:
| Scenario | Where |
|---|---|
sponsoredDetails() with gasToken returns { mode: "default", gasToken } |
wallet-utils.test.ts |
sponsoredDetails() without gasToken returns { mode: "sponsored" } |
wallet-utils.test.ts |
Wallet.execute({ gasToken, feeMode: "user_pays" }) throws |
wallet.test.ts |
Wallet.deploy({ gasToken, feeMode: "user_pays" }) throws |
wallet.test.ts |
gasToken without explicit feeMode routes to paymaster |
wallet.test.ts |
CartridgeWallet.deploy({ gasToken }) throws |
cartridge.test.ts |
CartridgeWallet.execute({ gasToken }) routes to paymaster |
cartridge.test.ts |
Note: sponsoredDetails() currently has zero test coverage (even before this PR), so adding baseline tests would be valuable.
Suggestions (non-blocking)
1. Extract shared base type for DeployOptions / ExecuteOptions
Both interfaces are now structurally identical. A shared base eliminates the JSDoc duplication and prevents future field drift:
interface TransactionFeeOptions {
feeMode?: FeeMode;
timeBounds?: PaymasterTimeBounds;
gasToken?: Address;
}
export interface DeployOptions extends TransactionFeeOptions {}
export interface ExecuteOptions extends TransactionFeeOptions {}2. Rename sponsoredDetails → paymasterDetails
The function now returns { mode: "default" } when gasToken is set, which is not "sponsored". paymasterDetails or buildPaymasterDetails would be accurate for both code paths.
3. Consider an options object for sponsoredDetails
The function now takes 3 positional optionals (timeBounds?, deploymentData?, gasToken?), which forces callers to pass undefined as a placeholder (e.g. sponsoredDetails(timeBounds, undefined, gasToken) in CartridgeWallet). An options object would be cleaner:
function paymasterDetails(options: {
timeBounds?: PaymasterTimeBounds;
deploymentData?: PAYMASTER_API.ACCOUNT_DEPLOYMENT_DATA;
gasToken?: Address;
} = {})4. Clarify gasToken JSDoc
The example (e.g. USDC, STRK) could be misread as token symbols. Since the type is Address, something like (e.g. the USDC or STRK contract address) would be clearer.
Also worth documenting the feeMode interaction — specifically that omitting feeMode with gasToken is the intended happy path, but combining feeMode: "user_pays" with gasToken throws.
5. ensureReady doesn't support gasToken
ensureWalletReady() calls wallet.deploy(feeMode ? { feeMode } : undefined) — no way to pass gasToken. If gasToken-paid deployment via ensureReady is a valid use case, EnsureReadyOptions should also get the gasToken field.
Overall this is a clean, well-scoped feature. The CartridgeWallet validation gap and test coverage are the two items I'd want addressed before merge. The rest are optional improvements. Nice work!
Bug fix: - Add gasToken conflict guard to CartridgeWallet.execute() — previously silently routed to paymaster when user_pays + gasToken were combined Refactoring: - Extract shared assertNoGasTokenConflict() to wallet/utils.ts (DRY) - Rename sponsoredDetails → paymasterDetails (reflects actual behavior) - Refactor positional args to options object pattern - Introduce TransactionFeeOptions base type (eliminates JSDoc duplication) - Add gasToken to EnsureReadyOptions with forwarding in ensureWalletReady - Clarify JSDoc: 'e.g. the USDC or STRK contract address' - Remove redundant Wallet.throwGasTokenConflict() private method Tests: - Add paymasterDetails unit tests (6 cases) - Add assertNoGasTokenConflict unit tests (5 cases) - Add ensureWalletReady gasToken forwarding tests (2 cases) - Add Wallet.execute/deploy gasToken conflict + routing tests (3 cases) - Add CartridgeWallet gasToken deploy guard, routing, conflict tests (3 cases) All 549 tests pass, 0 TypeScript errors.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/wallet/utils.ts (1)
72-96:⚠️ Potential issue | 🟠 MajorKeep
preflightTransaction()in sync with the newgasTokenpaymaster path.
paymasterDetails()now treatsgasTokenas another paymaster-capable mode, butpreflightTransaction()still only returns{ ok: true }for undeployed accounts whenfeeMode === "sponsored". A caller can now get"Account not deployed"fromwallet.preflight()for the same call thatwallet.execute(..., { gasToken })would send via paymaster. Please extendPreflightOptionswithgasTokenand handle it the same way here, plus add an undeployed-account regression test.♻️ Suggested follow-up
// src/types/wallet.ts export interface PreflightOptions { calls: Call[]; feeMode?: FeeMode; + gasToken?: Address; } // src/wallet/utils.ts - const { calls, feeMode } = options; + const { calls, feeMode, gasToken } = options; @@ - if (feeMode === "sponsored") { + if (feeMode === "sponsored" || gasToken !== undefined) { return { ok: true }; }As per coding guidelines,
src/**: This is a public SDK. Prioritize: Backwards compatibility and clear, typed public APIs.Also applies to: 154-188
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/wallet/utils.ts` around lines 72 - 96, preflightTransaction currently ignores gasToken and only treats feeMode === "sponsored" as a valid paymaster path for undeployed accounts; update the PreflightOptions type to include gasToken and modify preflightTransaction to treat a provided gasToken the same as feeMode === "sponsored" (i.e., return { ok: true } for undeployed accounts when gasToken is present), keeping logic consistent with paymasterDetails and wallet.execute (reference preflightTransaction, PreflightOptions, paymasterDetails, wallet.preflight, wallet.execute, gasToken); also add a regression test that calls wallet.preflight on an undeployed account with gasToken to assert it does not return "Account not deployed".
♻️ Duplicate comments (1)
src/wallet/index.ts (1)
359-369:⚠️ Potential issue | 🟠 MajorReject
gasTokenin the Braavos bootstrap path.
deployBraavosViaFactory()still sends the paymaster transaction from the temporary OpenZeppelin bootstrap account and forwardsgasTokentogether withozDeploymentData. That makes the ERC-20 fee target the bootstrap account, not the final Braavos wallet exposed by this instance, sowallet.deploy({ gasToken })and the firstwallet.execute(..., { gasToken })on undeployed Braavos wallets can fail even when the Braavos address is funded.
⚠️ Suggested fixprivate async deployBraavosViaFactory( calls: Call[], timeBounds?: PaymasterTimeBounds, gasToken?: Address ): Promise<Tx> { + if (gasToken !== undefined) { + throw new Error( + "Braavos deployment does not support gasToken because the paymaster transaction is sent from a temporary OpenZeppelin bootstrap account." + ); + } + const publicKey = await this.accountProvider.getPublicKey(); const signer = this.accountProvider.getSigner(); @@ await ozProvider.getDeploymentData(); const { transaction_hash } = await ozAccount.executePaymasterTransaction( allCalls, paymasterDetails({ timeBounds: timeBounds ?? this.defaultTimeBounds, deploymentData: ozDeploymentData, - gasToken, }) );In starknet.js 9.2.1 / AVNU Paymaster, when Account.executePaymasterTransaction uses feeMode { mode: "default", gasToken } with deploymentData for an undeployed account, which account is debited for the ERC-20 fee: the bootstrap/sender account from deploymentData or an account later deployed by a factory call in the same transaction?Also applies to: 398-401, 480-486
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/wallet/index.ts` around lines 359 - 369, The Braavos bootstrap path must not forward a gasToken because deployBraavosViaFactory currently sends the ERC-20 fee target to the temporary OZ bootstrap account; modify deployPaymasterWith so that when classHash === BraavosPreset.classHash any provided gasToken is rejected (throw a clear error) or explicitly cleared before calling deployBraavosViaFactory, and update any other Braavos bootstrap flows that forward ozDeploymentData (references: deployPaymasterWith, deployBraavosViaFactory, BraavosPreset.classHash) to ensure gasToken is never included in the bootstrap deploymentData/ozDeploymentData; apply the same change to the other Braavos handling spots mentioned (the other deploy/paymaster/bootstrap methods around the previously noted locations).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/wallet/utils.ts`:
- Around line 72-96: preflightTransaction currently ignores gasToken and only
treats feeMode === "sponsored" as a valid paymaster path for undeployed
accounts; update the PreflightOptions type to include gasToken and modify
preflightTransaction to treat a provided gasToken the same as feeMode ===
"sponsored" (i.e., return { ok: true } for undeployed accounts when gasToken is
present), keeping logic consistent with paymasterDetails and wallet.execute
(reference preflightTransaction, PreflightOptions, paymasterDetails,
wallet.preflight, wallet.execute, gasToken); also add a regression test that
calls wallet.preflight on an undeployed account with gasToken to assert it does
not return "Account not deployed".
---
Duplicate comments:
In `@src/wallet/index.ts`:
- Around line 359-369: The Braavos bootstrap path must not forward a gasToken
because deployBraavosViaFactory currently sends the ERC-20 fee target to the
temporary OZ bootstrap account; modify deployPaymasterWith so that when
classHash === BraavosPreset.classHash any provided gasToken is rejected (throw a
clear error) or explicitly cleared before calling deployBraavosViaFactory, and
update any other Braavos bootstrap flows that forward ozDeploymentData
(references: deployPaymasterWith, deployBraavosViaFactory,
BraavosPreset.classHash) to ensure gasToken is never included in the bootstrap
deploymentData/ozDeploymentData; apply the same change to the other Braavos
handling spots mentioned (the other deploy/paymaster/bootstrap methods around
the previously noted locations).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 55a419e8-aa60-432c-8866-606a6239fed3
📒 Files selected for processing (7)
src/types/wallet.tssrc/wallet/cartridge.tssrc/wallet/index.tssrc/wallet/utils.tstests/cartridge.test.tstests/wallet-utils.test.tstests/wallet.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: examples
- GitHub Check: test
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use@/path alias for internal imports in TypeScript source files
Use strict TypeScript compiler settings:strict,noUncheckedIndexedAccess,exactOptionalPropertyTypesenabled
Files:
tests/wallet.test.tstests/cartridge.test.tssrc/wallet/cartridge.tstests/wallet-utils.test.tssrc/wallet/utils.tssrc/wallet/index.tssrc/types/wallet.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not introduce CommonJS patterns; use ESM only
Files:
tests/wallet.test.tstests/cartridge.test.tssrc/wallet/cartridge.tstests/wallet-utils.test.tssrc/wallet/utils.tssrc/wallet/index.tssrc/types/wallet.ts
tests/**
⚙️ CodeRabbit configuration file
tests/**: Integration tests can be flaky depending on devnet/RPC assumptions.
Prefer stable unit tests unless an integration test is strictly necessary.
Files:
tests/wallet.test.tstests/cartridge.test.tstests/wallet-utils.test.ts
src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Preserve strict TypeScript typing; prefer explicit domain types (Address, Amount, ChainId) over primitives
Files:
src/wallet/cartridge.tssrc/wallet/utils.tssrc/wallet/index.tssrc/types/wallet.ts
{src/sdk.ts,src/wallet/**,src/types/**}
📄 CodeRabbit inference engine (AGENTS.md)
Must serialize: Concurrent edits in
src/sdk.ts,src/wallet/*, or sharedsrc/types/*
Files:
src/wallet/cartridge.tssrc/wallet/utils.tssrc/wallet/index.tssrc/types/wallet.ts
src/**
⚙️ CodeRabbit configuration file
src/**: This is a public SDK. Prioritize:
- Backwards compatibility and clear, typed public APIs
- Deterministic behavior across mainnet/sepolia presets
- Tests for behavior changes (prefer unit tests)
Files:
src/wallet/cartridge.tssrc/wallet/utils.tssrc/wallet/index.tssrc/types/wallet.ts
🧠 Learnings (10)
📓 Common learnings
Learnt from: adrienlacombe
Repo: keep-starknet-strange/starkzap PR: 62
File: src/tx/builder.ts:449-539
Timestamp: 2026-03-09T13:39:45.086Z
Learning: In `src/tx/builder.ts` and `src/confidential/types.ts` (StarkZap SDK, TypeScript), `sender` in confidential/tongo operations (ConfidentialFundDetails, ConfidentialTransferDetails, ConfidentialWithdrawDetails, ConfidentialRagequitDetails, ConfidentialRolloverDetails) refers to the Starknet account that submits the transaction on-chain, which is often a relayer — NOT necessarily the wallet owner. The optional `feeTo` field exists specifically for relayed transactions where `sender` differs from the wallet. Do NOT suggest overriding `details.sender` with `this.wallet.address` in TxBuilder confidential methods, as this would break the relayer pattern. By contrast, staking methods correctly bind `this.wallet.address` because staking requires the pool member's own address with no relayer concept.
📚 Learning: 2026-03-06T10:04:28.285Z
Learnt from: CR
Repo: keep-starknet-strange/starkzap PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-06T10:04:28.285Z
Learning: Applies to {src/sdk.ts,src/wallet/**,src/types/**} : Must serialize: Concurrent edits in `src/sdk.ts`, `src/wallet/*`, or shared `src/types/*`
Applied to files:
tests/wallet.test.tstests/wallet-utils.test.tssrc/wallet/utils.ts
📚 Learning: 2026-03-09T13:39:45.086Z
Learnt from: adrienlacombe
Repo: keep-starknet-strange/starkzap PR: 62
File: src/tx/builder.ts:449-539
Timestamp: 2026-03-09T13:39:45.086Z
Learning: In `src/tx/builder.ts` and `src/confidential/types.ts` (StarkZap SDK, TypeScript), `sender` in confidential/tongo operations (ConfidentialFundDetails, ConfidentialTransferDetails, ConfidentialWithdrawDetails, ConfidentialRagequitDetails, ConfidentialRolloverDetails) refers to the Starknet account that submits the transaction on-chain, which is often a relayer — NOT necessarily the wallet owner. The optional `feeTo` field exists specifically for relayed transactions where `sender` differs from the wallet. Do NOT suggest overriding `details.sender` with `this.wallet.address` in TxBuilder confidential methods, as this would break the relayer pattern. By contrast, staking methods correctly bind `this.wallet.address` because staking requires the pool member's own address with no relayer concept.
Applied to files:
tests/wallet.test.ts
📚 Learning: 2026-03-06T10:04:28.285Z
Learnt from: CR
Repo: keep-starknet-strange/starkzap PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-06T10:04:28.285Z
Learning: Applies to {src/erc20/token/presets*.ts,src/staking/validator/presets*.ts} : Must serialize: Generated artifact updates in `src/erc20/token/presets*.ts` and `src/staking/validator/presets*.ts`
Applied to files:
tests/wallet.test.tstests/cartridge.test.tstests/wallet-utils.test.tssrc/wallet/utils.ts
📚 Learning: 2026-03-17T08:43:15.273Z
Learnt from: ZackAttax
Repo: keep-starknet-strange/starkzap PR: 86
File: packages/native/src/cartridge/ts/outside_execution_v3.ts:36-38
Timestamp: 2026-03-17T08:43:15.273Z
Learning: In `packages/native/src/cartridge/ts/outside_execution_v3.ts`, the `SESSION_TYPE_HASH` encodes only 4 fields ("Expires At", "Allowed Methods", "Metadata", "Session Key") while `hashSessionStruct` serializes 5 fields (adding `guardianKeyGuid`). This mismatch is intentional and inherited from the upstream Cartridge protocol (controller-rs / controller.c session token shape), where the same 4-field type-hash / 5-field payload pattern exists. Changing starkzap alone would break protocol parity and invalidate outside-execution signatures against live Cartridge contracts. This should only be fixed upstream as a coordinated/versioned protocol change.
Applied to files:
tests/cartridge.test.tssrc/wallet/cartridge.ts
📚 Learning: 2026-03-06T10:04:13.920Z
Learnt from: CR
Repo: keep-starknet-strange/starkzap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-06T10:04:13.920Z
Learning: Applies to src/**/*.ts : Preserve strict TypeScript typing; prefer explicit domain types (Address, Amount, ChainId) over primitives
Applied to files:
tests/cartridge.test.ts
📚 Learning: 2026-03-06T10:04:13.920Z
Learnt from: CR
Repo: keep-starknet-strange/starkzap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-06T10:04:13.920Z
Learning: Applies to src/erc20/token/presets*.ts : Do not hand-edit auto-generated preset files: `src/erc20/token/presets.ts`, `src/erc20/token/presets.sepolia.ts`, `src/staking/validator/presets.ts`, `src/staking/validator/presets.sepolia.ts`
Applied to files:
tests/cartridge.test.ts
📚 Learning: 2026-03-09T13:42:08.400Z
Learnt from: adrienlacombe
Repo: keep-starknet-strange/starkzap PR: 62
File: tests/integration/confidential.test.ts:44-87
Timestamp: 2026-03-09T13:42:08.400Z
Learning: In `tests/integration/confidential.test.ts` (StarkZap, TypeScript/Vitest), the Confidential integration test suite intentionally spawns its own dedicated devnet (port 5110, loading `tongo-state/devnet.state`) instead of using the shared globalSetup devnet. This is because it requires a pre-built state dump with an already-deployed Tongo contract whose artifacts come from a separate repo and are baked in via `regenerate.mjs`. The shared devnet starts fresh (--seed 0, no contracts). The graceful warn+skip on devnet startup failure is intentional: if starknet-devnet is not installed or the state dump is missing, other tests should still pass. This is the accepted pattern for optional integration suites depending on external tooling in this project.
Applied to files:
tests/cartridge.test.tstests/wallet-utils.test.ts
📚 Learning: 2026-03-09T13:39:32.603Z
Learnt from: adrienlacombe
Repo: keep-starknet-strange/starkzap PR: 62
File: src/tx/builder.ts:449-539
Timestamp: 2026-03-09T13:39:32.603Z
Learning: Guideline for confidential transaction handling in StarkZap: In src/tx/builder.ts and src/confidential/types.ts, the sender field for confidential/tongo operations (ConfidentialFundDetails, ConfidentialTransferDetails, ConfidentialWithdrawDetails, ConfidentialRagequitDetails, ConfidentialRolloverDetails) represents the Starknet account submitting the transaction (often a relayer), not necessarily the wallet owner. The optional feeTo field is for relayed transactions where sender differs from the wallet.
Actionable checks for code reviews:
- Do not override details.sender with this.wallet.address in TxBuilder confidential methods; doing so breaks the relayer pattern.
- Ensure staking methods continue to bind this.wallet.address to reflect the pool member’s own address (no relayer concept there).
- If using a relayer, verify feeTo is set when appropriate and that sender remains the relayer address.
- Update or add tests to reflect the correct sender semantics for confidential operations...
Applied to files:
src/wallet/cartridge.tssrc/wallet/utils.tssrc/wallet/index.tssrc/types/wallet.ts
📚 Learning: 2026-03-09T13:38:38.248Z
Learnt from: adrienlacombe
Repo: keep-starknet-strange/starkzap PR: 62
File: src/confidential/confidential.ts:104-145
Timestamp: 2026-03-09T13:38:38.248Z
Learning: In `src/confidential/confidential.ts` (and by extension the broader StarkZap SDK), the convention is that `Amount.toBase()` is trusted inside `populate*` methods — no token/decimal metadata validation is performed within the method. This is consistent with ERC20 and Staking modules. `feeTo` in `ConfidentialFundDetails`, `ConfidentialTransferDetails`, etc. is a raw `bigint`, not an `Amount`, and goes straight to the tongo-sdk without conversion.
Applied to files:
src/wallet/index.ts
preflightTransaction() now returns { ok: true } for undeployed accounts
when gasToken is set, matching the paymaster path behavior in execute().
Previously it would return a false 'Account not deployed' error.
- Add gasToken to PreflightOptions interface
- Update preflightTransaction to treat gasToken like sponsored mode
- Add regression test for undeployed account + gasToken preflight
|
@0xLucqs Thanks for the review! All feedback has been addressed:
All 550 tests passing. |
micbakos
left a comment
micbakos
left a comment
There was a problem hiding this comment.
@ahmetenesdur My only remark regards to the structure of the input of FeeMode. Gas token can only be used with paymaster. So it is possible a user to provide feeMode: "user_pays", gasToken: "0x...". In this case gasToken will be ignored at runtime but again it implies that you can pay any tx with any gas token. So instead I would argue that we could improve on the design of the type like so:
/**
* How transaction fees are paid.
* - `{ type: "paymaster" }`: Paymaster covers gas with its default token
* - `{ type: "paymaster", gasToken }`: Paymaster uses the specified gas token
* - `"user_pays"`: User's account pays gas in ETH/STRK
* - `"sponsored"`: Deprecated alias for paymaster-covered gas
*/
/**
* @deprecated Use `{ type: "paymaster" }` instead.
*/
type DeprecatedSponsoredFeeMode = "sponsored";
export type FeeMode =
| DeprecatedSponsoredFeeMode
| {
type: "paymaster";
gasToken?: Token | Address | undefined;
}
| "user_pays";
Now when "paymaster" is defined you can optionally attach a gasToken token or address or ignore it completely. Keep in mind that I kept "sponsored" as a possible type but deprecated so as not to break compatibility with older versions.
|
@micbakos Good point — making the invalid state unrepresentable at the type level is cleaner than a runtime guard. A couple of things to consider though:
The current runtime guard ( |
I am not fully convinced that this is the correct approach. This is a higher level library, and not a client built to interface with starknet rpc directly. There is no |
|
@micbakos You're right — |
Replace the separate gasToken field with a discriminated FeeMode union
per maintainer review (micbakos):
Type changes:
- FeeMode is now "user_pays" | { type: "paymaster"; gasToken?: Address } | "sponsored"
- Remove standalone gasToken from DeployOptions, ExecuteOptions, PreflightOptions
- Keep "sponsored" as deprecated backward-compatible alias
Internal routing:
- Add normalizeFeeMode() to convert "sponsored" to { type: "paymaster" }
- Add isPaymasterMode() type guard for preflight logic
- Remove assertNoGasTokenConflict() — type system prevents invalid combos
- Rename local variable in paymasterDetails() to avoid shadowing
Packages:
- native: widen SupportedNativeCartridgeFeeMode, add null-safe validation
- mcp-server: use canonical { type: "paymaster" } form
- examples: update flappy-bird; web/mobile unchanged (deprecated alias works)
Tests: add 11 new tests for normalization, type guard and backward compat.
All 740 tests pass across root, native and mcp-server packages.
|
@micbakos Refactor pushed in FeeMode is now Removed |
- Extract deprecated "sponsored" into DeprecatedSponsoredFeeMode type alias with @deprecated JSDoc tag (micbakos) - Narrow SupportedNativeCartridgeFeeMode to exclude gasToken and reject it at validation time since Cartridge sessions only support sponsored execution (0xLucqs) All 740 tests pass. Lint and formatting clean.
3 participants
Closes #118
What
Adds an optional
gasTokenfield toExecuteOptionsandDeployOptions, allowing users to pay gas fees with ERC-20 tokens (USDC, STRK, etc.) through the AVNU Paymaster.When
gasTokenis set, the SDK callsexecutePaymasterTransactionwith{ mode: 'default', gasToken }instead of{ mode: 'sponsored' }. When omitted, nothing changes.Changes
src/types/wallet.tsgasToken?: AddresstoExecuteOptionsandDeployOptionssrc/wallet/utils.tssponsoredDetails()now acceptsgasTokenand produces the correct fee modesrc/wallet/index.tsexecute()anddeploy()read and forwardgasTokenthrough the full chainsrc/wallet/cartridge.tsexecute()forwardsgasToken;deploy()guards against itUsage
Tests
All 116 unit tests pass. No breaking changes.