fix(generic-delegator): correct gas estimation for smart-account txs#623
Merged
Conversation
The generic delegator's /estimate/clauses endpoint simulates the user's raw clauses without the executeWithAuthorization wrapper and without the embedded-wallet typed-data signature, so it under-estimates gas (and for NFT-heavy clauses can revert outright in simulation). The returned transactionCost was then used to build the transfer clause that pays the delegator's deposit account, leaving it insufficient to cover the actual on-chain gas. The failure was masked by a catch-all that surfaced a misleading "no gas tokens have sufficient balance" error even when the wallet had ample balance. Mirror VeWorld mobile's approach: trust the delegator only for the gas-token-per-gas rate (a market price independent of the gas amount), recompute the gas number locally with thor.gas.estimateGas (caller = smart account), add a fixed wrapper overhead per smart-account version plus a fixed fee-payer overhead per gas token, apply a 1.1 safety multiplier, then reapply the rate. New shared helper computeCorrectedGasTokenCost is used by the send path, the fee-estimation UI hook, and the all-tokens hook so they all agree. Also revert the homepage/playground feeDelegation override introduced in the cross-app PR. It set feeDelegation.delegatorUrl (dApp-sponsored mode) to the generic-delegator URL, which routed transactions through the dapp-kit fee-delegator path that POSTs to the bare base URL and 404s. With no override the kit's default getGenericDelegatorUrl() takes over and the request hits the correct /api/v1/sign/transaction/authorized/... endpoint. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughPrecomputes corrected local gas (with timeout, wrapper overhead, safety multiplier), converts it to per-token costs via delegator rate data, uses corrected cost for deposit transfers and fee-estimated flows, injects kit-sponsored delegator URLs in provider defaults, removes example-level feeDelegation, and decodes delegator deposit transfers as fee payments with localized labels. ChangesGeneric Delegator Gas Correction and Fee Estimation
Sequence DiagramsequenceDiagram
participant UI as UI (Transact flow)
participant Transact as TransactClient
participant Decoder as decodeClause
participant DelegatorAPI as GenericDelegator API
participant Thor as ThorClient
participant DelegatorDeposit as Delegator deposit/account
UI->>Transact: submit/preview transaction clauses
Transact->>DelegatorAPI: (once) fetch generic delegator deposit account
DelegatorAPI-->>Transact: depositAccount (cached) or null
Transact->>Decoder: decodeClause(clause, thor, network, self, feeDepositAccount)
Decoder-->>Transact: known_action / token/native transfer (labels 'fee' when matches depositAccount)
Transact->>Thor: simulate local gas (computeCorrectedTotalGasNoFeePayer)
Thor-->>Transact: local gas estimate or error/null
Transact->>DelegatorAPI: /estimate/clauses (rate data) for convertGasToGasTokenAmount
DelegatorAPI-->>Transact: rates
Transact->>DelegatorAPI: sendTransaction(clauses(), delegatorUrl) // when routing via delegator
DelegatorAPI-->>DelegatorDeposit: deposit/account receives fund (fee payment)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate 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 |
|
Size Change: +41.9 kB (+0.48%) Total Size: 8.84 MB
ℹ️ View Unchanged
|
… timeout The fee-estimation UI hooks (useGenericDelegatorFeeEstimation, useEstimateAllTokens) were calling thor.gas.estimateGas INSIDE the per-token loop, multiplying the local RPC round-trips by the number of candidate gas tokens. Worse, if a single estimate hung — slow mainnet RPC, unusual clause shape — the entire queryFn never resolved and the "Confirm transfer" page sat on a spinner forever. Split computeCorrectedGasTokenCost into two helpers: - computeCorrectedTotalGasNoFeePayer: runs the local Thor estimate once, bounded by a 6s timeout (returns null on timeout/failure so callers can fall back to a delegator-derived value). - convertGasToGasTokenAmount: pure function, applies the per-token fee-payer overhead and the delegator's per-gas rate. Both fee-estimation hooks now call the gas helper once outside the loop, then convert per-token inside. computeCorrectedGasTokenCost remains a thin wrapper over the two, used by the send path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
useGenericDelegatorFeeEstimation and useEstimateAllTokens are restored to their pre-PR (2.9.0) behavior: they still call the delegator's /estimate/clauses endpoint and use the returned transactionCost as-is for the balance check. Reason: routing the local thor.gas.estimateGas through these hooks made the Confirm-transfer page sit on a spinner without ever firing the delegator network call. The send path keeps the locally-corrected gas-token amount (via computeCorrectedGasTokenCost) so on-chain transfers still cover the actual cost; the UI just shows the (occasionally under-estimated) value the delegator returns. Trade-off accepted for now: fee UI and on-chain transfer amount may diverge slightly. Will revisit once the upstream regression in #620 (host Privy app id swap + cross-app session shape) is sorted out. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Temporary console.log in useEstimateAllTokens and useGenericDelegatorFeeEstimation to confirm which input is making the react-query disabled (status=pending, fetchStatus=idle). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/vechain-kit/src/hooks/generic-delegator/useEstimateAllTokens.ts`:
- Around line 24-32: Remove the runtime console.log in useEstimateAllTokens (the
debug logging that prints callerEnabled, clauses.length, smartAccount?.address,
feeDelegation?.genericDelegatorUrl, feeDelegation?.delegatorUrl, and
tokens.length) or gate it so it only runs in development; specifically, delete
the console.log call in the useEstimateAllTokens hook or wrap it behind a
development-only check (e.g., NODE_ENV === 'development' or a dedicated debug
flag) so no PII or endpoint details are logged in production.
In `@packages/vechain-kit/src/hooks/generic-delegator/useGenericDelegator.ts`:
- Around line 271-290: The current logic returns a potentially non-positive
fallbackCost and may return non-positive cost; update the function in
useGenericDelegator (around fallbackCost, computeCorrectedTotalGasNoFeePayer and
convertGasToGasTokenAmount usage) to validate that both the computed cost and
fallbackCost are strictly positive before returning: if totalGasNoFeePayer is
null, compute fallbackCost and if fallbackCost <= 0 (use BigInt comparisons)
throw/reject with a clear error; likewise after computing cost from
convertGasToGasTokenAmount, if cost <= 0 throw/reject and do not return a
non-positive value. Ensure you use BigInt for amount checks and keep existing
flow around computeCorrectedTotalGasNoFeePayer and estimationResponse intact.
In
`@packages/vechain-kit/src/hooks/generic-delegator/useGenericDelegatorFeeEstimation.ts`:
- Around line 33-43: Remove the unconditional debug console.log in the
useGenericDelegatorFeeEstimation hook (the block logging '[vk-debug]
useGenericDelegatorFeeEstimation inputs:') because it leaks account and endpoint
metadata; either delete that console.log entirely or wrap it with a
development-only guard (e.g., process.env.NODE_ENV === 'development' or a isDev
flag) so it only runs in non-production builds, keeping the rest of the hook
(enabled, clauses, account, smartAccount, feeDelegation, tokens, balances)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 96741040-c5cb-4eca-9bd1-4376726a566e
📒 Files selected for processing (5)
examples/homepage/src/app/providers/VechainKitProviderWrapper.tsxexamples/playground/src/app/providers/VechainKitProviderWrapper.tsxpackages/vechain-kit/src/hooks/generic-delegator/useEstimateAllTokens.tspackages/vechain-kit/src/hooks/generic-delegator/useGenericDelegator.tspackages/vechain-kit/src/hooks/generic-delegator/useGenericDelegatorFeeEstimation.ts
💤 Files with no reviewable changes (1)
- examples/homepage/src/app/providers/VechainKitProviderWrapper.tsx
Comment on lines
+271
to
+290
| const fallbackCost = (estimationResponse.transactionCost ?? 0) * 2; | ||
|
|
||
| const totalGasNoFeePayer = await computeCorrectedTotalGasNoFeePayer({ | ||
| thor, | ||
| clauses, | ||
| smartAccountAddress, | ||
| version, | ||
| timeoutMs, | ||
| }); | ||
| if (totalGasNoFeePayer === null) { | ||
| return fallbackCost; | ||
| } | ||
|
|
||
| const cost = convertGasToGasTokenAmount({ | ||
| totalGasNoFeePayer, | ||
| gasToken, | ||
| estimationResponse, | ||
| }); | ||
| return cost > 0 ? cost : fallbackCost; | ||
| }; |
There was a problem hiding this comment.
Fail fast when fallback cost is non-positive.
fallbackCost can become 0 and propagate to a zero funding transfer, which then fails later with a misleading insufficient-balance path. Reject non-positive computed/fallback costs here instead of returning them.
Proposed fix
export const computeCorrectedGasTokenCost = async ({
@@
}): Promise<number> => {
const fallbackCost = (estimationResponse.transactionCost ?? 0) * 2;
@@
if (totalGasNoFeePayer === null) {
- return fallbackCost;
+ if (fallbackCost > 0) return fallbackCost;
+ throw new Error('Unable to compute a positive fallback gas-token cost');
}
@@
const cost = convertGasToGasTokenAmount({
totalGasNoFeePayer,
gasToken,
estimationResponse,
});
- return cost > 0 ? cost : fallbackCost;
+ if (cost > 0) return cost;
+ if (fallbackCost > 0) return fallbackCost;
+ throw new Error('Unable to compute a positive gas-token cost');
};As per coding guidelines, "Always validate inputs in transaction and contract interaction functions - check address validity with isAddress() and ensure amounts are positive using BigInt comparisons".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/vechain-kit/src/hooks/generic-delegator/useGenericDelegator.ts`
around lines 271 - 290, The current logic returns a potentially non-positive
fallbackCost and may return non-positive cost; update the function in
useGenericDelegator (around fallbackCost, computeCorrectedTotalGasNoFeePayer and
convertGasToGasTokenAmount usage) to validate that both the computed cost and
fallbackCost are strictly positive before returning: if totalGasNoFeePayer is
null, compute fallbackCost and if fallbackCost <= 0 (use BigInt comparisons)
throw/reject with a clear error; likewise after computing cost from
convertGasToGasTokenAmount, if cost <= 0 throw/reject and do not return a
non-positive value. Ensure you use BigInt for amount checks and keep existing
flow around computeCorrectedTotalGasNoFeePayer and estimationResponse intact.
The old gate (`privy !== undefined || loginMethods includes 'vechain'/'ecosystem'`) left `feeDelegation` undefined whenever a consumer only listed Google/Apple/Twitter/etc. without a `privy` prop. After #620 those buttons silently fall back to VeChain's whitelabel cross-app host, so the user ends up on a smart-account wallet that needs fee delegation — but `useGenericDelegatorFeeEstimation` and `useEstimateAllTokens` stay permanently disabled because they require `feeDelegation?.genericDelegatorUrl`, and the spinner on the Confirm page never resolves. Drop the gate: always seed `genericDelegatorUrl` when the consumer didn't pick a delegation strategy. dapp-kit wallets ignore it, smart-account users get a working default. Also drops the temporary debug console.logs from the two hooks. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reapplies the original plan after the auto-inject regression in VeChainKitProvider was fixed: useGenericDelegatorFeeEstimation and useEstimateAllTokens now compute the gas-token cost via computeCorrectedTotalGasNoFeePayer (one local thor.gas.estimateGas, bounded by a 6s timeout, gas-token-agnostic) plus convertGasToGasTokenAmount (delegator-derived per-gas rate + per-token fee-payer overhead). This keeps the fee shown in the Confirm-transfer UI consistent with the value the send path actually transfers to the generic delegator's deposit account, so the balance check and the on-chain transfer agree. If the local estimate times out or fails, fall back to `delegator.transactionCost * 2` so the UI stays responsive even when the Thor node is slow or the simulation reverts. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…saction fee" The transact page shows every clause the user is asked to sign. When the requester routes through the generic delegator, one of those clauses is a VET / VTHO / B3TR / VOT3 transfer to the delegator's deposit account — previously rendered as "Send 0.21 VET to 0x86…c6fa", which a non- crypto user reads as a separate transfer rather than a gas-payment side effect. Resolve the delegator's current deposit account once per page load (cached via fetchGenericDelegatorDepositAccount) and pass it to the decoder. Clauses whose recipient matches the deposit account are now emitted as a known_action with the new `fee` category, summary "Pay transaction fee" / "Paga fee per transazione", and a detail line showing the amount + symbol routed to the gas payer. If the delegator endpoint is unreachable, fetchGenericDelegatorDepositAccount returns null and the decoder falls back to the original Send-style label — no regression for offline / down delegator. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
VeChain now sponsors the gas for first-time onboarding flows so users without VTHO / B3TR / VET can still complete them. Routes claim-domain, claim-veworld-subdomain, and update-text-record transactions through the new kit-sponsored vechain.energy delegator: - mainnet: https://sponsor.vechain.energy/by/1060 - testnet: https://sponsor-testnet.vechain.energy/by/221 Surfaces a new `getKitSponsoredDelegatorUrl()` helper alongside the existing `getGenericDelegatorUrl()`. The three hooks pass the sponsored URL as the per-transaction `delegationUrl` override, so the rest of the kit's delegation plumbing (PrivyWalletProvider.signAndSend) routes through the dApp-sponsored path automatically. ChooseNameSummaryContent and CustomizationSummaryContent gate the gas-token UI on a new `KIT_PAYS_GAS = true` flag: the useGenericDelegatorFeeEstimation call is disabled, the GasFeeSummary section hidden, "has enough gas balance" is forced true, and the Confirm button is no longer waiting for an estimate. The user just sees the action confirmation; no fee disclosure since they pay nothing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Mobile Safari zooms the page whenever a focused input's computed font-size is below 16px CSS pixels. The kit's `md` font token resolves to 14px (see tokens.ts:573 — small=12, medium=14, large=16), so Chakra's default Input/Textarea inherits 14px through fontSize="md" and triggers the zoom on every form in the modal (Customization, SendToken, SendNft, Swap, FAQ search, etc). New theme/input.ts registers Input and Textarea component themes that pin field font-size to a hard 16px. Anchored in absolute pixels rather than `lg` so future token tweaks (e.g. shrinking large to 15px) can't silently regress this. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
packages/vechain-kit/src/hooks/generic-delegator/useEstimateAllTokens.ts (1)
96-101: 💤 Low valueConsider gating on
smartAccountVersionto avoid premature estimation.The query can run before
useGetAccountVersionresolves, usingversion: 0as fallback. This may produce an estimate with incorrect wrapper overhead, then re-run when the version loads.Proposed fix
enabled: enabled && clauses.length > 0 && !!smartAccount?.address && + smartAccountVersion !== undefined && !!feeDelegation?.genericDelegatorUrl && tokens.length > 0,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/vechain-kit/src/hooks/generic-delegator/useEstimateAllTokens.ts` around lines 96 - 101, The enabled condition for the estimation query in useEstimateAllTokens.ts can run before the account version resolves and use a fallback version (0); update the query's enabled predicate to also require the account version to be resolved (e.g., check smartAccountVersion or a resolved flag from useGetAccountVersion) so that estimation only runs when smartAccountVersion is available, preventing premature estimates and unnecessary re-runs of the estimate call.cross-app-connect/src/app/cross-app/transact/TransactClient.tsx (1)
31-37: ⚡ Quick winUse src path aliases for the new import.
Line 31 introduces a relative import inside
src, which breaks the repo import-path convention.As per coding guidelines, "/src//*.{ts,tsx}: Use path aliases for imports:
@/*for src root,@hooksfor hooks,@componentsfor components,@utilsfor utils".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cross-app-connect/src/app/cross-app/transact/TransactClient.tsx` around lines 31 - 37, The import of fetchGenericDelegatorDepositAccount, getChainId, getSmartAccountAddress, networkType, thor, and SmartAccountInfo is using a relative path ('../_lib/thor') inside src; change it to the src path alias form (e.g. import { fetchGenericDelegatorDepositAccount, getChainId, getSmartAccountAddress, networkType, thor, type SmartAccountInfo } from '`@/app/cross-app/_lib/thor`') so the module uses the repository's "`@/`..." alias convention for imports.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cross-app-connect/src/app/cross-app/_lib/decoder.ts`:
- Around line 182-193: The branch that treats any ERC‑20 transfer to the deposit
account as a fee must be guarded by a whitelist of known gas token addresses;
update the condition in the decoder (the block using isFeeDeposit(recipient)
that returns kind: 'known_action', category: 'fee') to also check that the token
being transferred is one of the supported fee tokens (e.g., via an
isKnownGasToken(token.address) helper or a constant set of addresses for
VTHO/B3TR/VOT3), handling casing/normalization and the possibility token or
token.address is undefined; only return the fee-labeled object when both
isFeeDeposit(recipient) and the gas-token whitelist check pass, otherwise fall
through to the normal transfer handling.
In
`@packages/vechain-kit/src/components/AccountModal/Contents/ChooseName/ChooseNameSummaryContent.tsx`:
- Around line 166-169: The code unconditionally sets KIT_PAYS_GAS = true in
ChooseNameSummaryContent which incorrectly disables fee gating for the
unset-domain flow (isUnsetting / useUnsetDomain); change KIT_PAYS_GAS from a
constant to a computed boolean that reflects the actual transaction path (e.g.,
KIT_PAYS_GAS = !isUnsetting && <existing sponsored-condition>), and update all
usages in ChooseNameSummaryContent (the places around the comments and the
blocks referenced at the diffs) so that when isUnsetting is true the component
shows the fee token UI and performs the VTHO/fee checks and error handling
instead of skipping them.
In `@packages/vechain-kit/src/hooks/generic-delegator/useEstimateAllTokens.ts`:
- Around line 31-36: The query key used in useEstimateAllTokens.ts for the
totalGasNoFeePayer computation omits the smartAccountVersion dependency, so
changes to smartAccountVersion?.version (from useGetAccountVersion) won't
invalidate the cached estimate; update the query key to include
smartAccountVersion?.version (or a stable representation like
`smartAccountVersion?.version ?? null`) alongside existing keys, and ensure the
totalGasNoFeePayer calculation still reads smartAccountVersion?.version so the
query will refetch when the account version changes.
In
`@packages/vechain-kit/src/hooks/generic-delegator/useGenericDelegatorFeeEstimation.ts`:
- Around line 46-47: The query key in useGenericDelegatorFeeEstimation is
missing the smartAccountVersion dependency so cache won't invalidate when
smartAccountVersion?.version changes; update the queryKey (the const queryKey
used for gas estimation) to include smartAccountVersion?.version (or the
smartAccountVersion object) alongside clauses, tokens, sendingAmount, and
sendingTokenSymbol so refetches occur when the smart account version updates.
---
Nitpick comments:
In `@cross-app-connect/src/app/cross-app/transact/TransactClient.tsx`:
- Around line 31-37: The import of fetchGenericDelegatorDepositAccount,
getChainId, getSmartAccountAddress, networkType, thor, and SmartAccountInfo is
using a relative path ('../_lib/thor') inside src; change it to the src path
alias form (e.g. import { fetchGenericDelegatorDepositAccount, getChainId,
getSmartAccountAddress, networkType, thor, type SmartAccountInfo } from
'`@/app/cross-app/_lib/thor`') so the module uses the repository's "`@/`..." alias
convention for imports.
In `@packages/vechain-kit/src/hooks/generic-delegator/useEstimateAllTokens.ts`:
- Around line 96-101: The enabled condition for the estimation query in
useEstimateAllTokens.ts can run before the account version resolves and use a
fallback version (0); update the query's enabled predicate to also require the
account version to be resolved (e.g., check smartAccountVersion or a resolved
flag from useGetAccountVersion) so that estimation only runs when
smartAccountVersion is available, preventing premature estimates and unnecessary
re-runs of the estimate call.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 166a41c2-ea96-4705-9c00-8e87ce976ffb
📒 Files selected for processing (16)
cross-app-connect/src/app/cross-app/_lib/decoder.tscross-app-connect/src/app/cross-app/_lib/knownActions.tscross-app-connect/src/app/cross-app/_lib/thor.tscross-app-connect/src/app/cross-app/transact/TransactClient.tsxcross-app-connect/src/app/i18n/locales/en.jsoncross-app-connect/src/app/i18n/locales/it.jsonpackages/vechain-kit/src/components/AccountModal/Contents/ChooseName/ChooseNameSummaryContent.tsxpackages/vechain-kit/src/components/AccountModal/Contents/Profile/Customization/CustomizationSummaryContent.tsxpackages/vechain-kit/src/constants/urls.tspackages/vechain-kit/src/hooks/api/vetDomains/useClaimVeWorldSubdomain.tspackages/vechain-kit/src/hooks/api/vetDomains/useClaimVetDomain.tspackages/vechain-kit/src/hooks/api/vetDomains/useUpdateTextRecord.tspackages/vechain-kit/src/hooks/generic-delegator/useEstimateAllTokens.tspackages/vechain-kit/src/hooks/generic-delegator/useGenericDelegatorFeeEstimation.tspackages/vechain-kit/src/providers/VeChainKitProvider.tsxpackages/vechain-kit/src/utils/constants.tsx
✅ Files skipped from review due to trivial changes (1)
- cross-app-connect/src/app/i18n/locales/en.json
Comment on lines
+182
to
+193
| if (isFeeDeposit(recipient)) { | ||
| return { | ||
| kind: 'known_action', | ||
| category: 'fee', | ||
| recipient, | ||
| summary: t('action.fee.payTransactionFee'), | ||
| detail: t('action.fee.amount', { | ||
| amount: trimAmount(amount), | ||
| symbol: token.symbol, | ||
| }), | ||
| }; | ||
| } |
There was a problem hiding this comment.
Restrict fee relabeling to supported gas tokens only.
Right now, any ERC‑20 transfer to the deposit account is shown as “Pay transaction fee”. That can hide a malicious transfer of an unrelated token. Gate this branch to known fee tokens (e.g., VTHO/B3TR/VOT3 by address) before returning category: 'fee'.
Suggested guard
- if (isFeeDeposit(recipient)) {
+ const isSupportedFeeToken =
+ ['VTHO', 'B3TR', 'VOT3'].includes(token.symbol.toUpperCase());
+ if (isFeeDeposit(recipient) && isSupportedFeeToken) {
return {
kind: 'known_action',
category: 'fee',📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (isFeeDeposit(recipient)) { | |
| return { | |
| kind: 'known_action', | |
| category: 'fee', | |
| recipient, | |
| summary: t('action.fee.payTransactionFee'), | |
| detail: t('action.fee.amount', { | |
| amount: trimAmount(amount), | |
| symbol: token.symbol, | |
| }), | |
| }; | |
| } | |
| const isSupportedFeeToken = | |
| ['VTHO', 'B3TR', 'VOT3'].includes(token.symbol.toUpperCase()); | |
| if (isFeeDeposit(recipient) && isSupportedFeeToken) { | |
| return { | |
| kind: 'known_action', | |
| category: 'fee', | |
| recipient, | |
| summary: t('action.fee.payTransactionFee'), | |
| detail: t('action.fee.amount', { | |
| amount: trimAmount(amount), | |
| symbol: token.symbol, | |
| }), | |
| }; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cross-app-connect/src/app/cross-app/_lib/decoder.ts` around lines 182 - 193,
The branch that treats any ERC‑20 transfer to the deposit account as a fee must
be guarded by a whitelist of known gas token addresses; update the condition in
the decoder (the block using isFeeDeposit(recipient) that returns kind:
'known_action', category: 'fee') to also check that the token being transferred
is one of the supported fee tokens (e.g., via an isKnownGasToken(token.address)
helper or a constant set of addresses for VTHO/B3TR/VOT3), handling
casing/normalization and the possibility token or token.address is undefined;
only return the fee-labeled object when both isFeeDeposit(recipient) and the
gas-token whitelist check pass, otherwise fall through to the normal transfer
handling.
Comment on lines
+31
to
+36
| const { data: smartAccountVersion } = useGetAccountVersion( | ||
| smartAccount?.address ?? '', | ||
| connectedWallet?.address ?? '', | ||
| ); | ||
| const { feeDelegation, network } = useVeChainKitConfig(); | ||
| const thor = ThorClient.at(getConfig(network.type).nodeUrl); |
There was a problem hiding this comment.
Query key missing smartAccountVersion dependency.
The totalGasNoFeePayer computation depends on smartAccountVersion?.version, but the query key doesn't include it. If the version changes after initial fetch (e.g., account upgrade), the cached estimate won't reflect the updated wrapper overhead.
Proposed fix
return useQuery({
queryKey: [
'gas-estimation-all-tokens',
JSON.stringify(clauses),
JSON.stringify(tokens),
+ smartAccountVersion?.version,
],📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const { data: smartAccountVersion } = useGetAccountVersion( | |
| smartAccount?.address ?? '', | |
| connectedWallet?.address ?? '', | |
| ); | |
| const { feeDelegation, network } = useVeChainKitConfig(); | |
| const thor = ThorClient.at(getConfig(network.type).nodeUrl); | |
| const { data: smartAccountVersion } = useGetAccountVersion( | |
| smartAccount?.address ?? '', | |
| connectedWallet?.address ?? '', | |
| ); | |
| const { feeDelegation, network } = useVeChainKitConfig(); | |
| const thor = ThorClient.at(getConfig(network.type).nodeUrl); | |
| return useQuery({ | |
| queryKey: [ | |
| 'gas-estimation-all-tokens', | |
| JSON.stringify(clauses), | |
| JSON.stringify(tokens), | |
| smartAccountVersion?.version, | |
| ], |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/vechain-kit/src/hooks/generic-delegator/useEstimateAllTokens.ts`
around lines 31 - 36, The query key used in useEstimateAllTokens.ts for the
totalGasNoFeePayer computation omits the smartAccountVersion dependency, so
changes to smartAccountVersion?.version (from useGetAccountVersion) won't
invalidate the cached estimate; update the query key to include
smartAccountVersion?.version (or a stable representation like
`smartAccountVersion?.version ?? null`) alongside existing keys, and ensure the
totalGasNoFeePayer calculation still reads smartAccountVersion?.version so the
query will refetch when the account version changes.
Comment on lines
46
to
47
| // Only include essential data in query key to prevent unnecessary refetches | ||
| const queryKey = ['gas-estimation', JSON.stringify(clauses), JSON.stringify(tokens), sendingAmount, sendingTokenSymbol]; |
There was a problem hiding this comment.
Query key missing smartAccountVersion dependency.
Similar to useEstimateAllTokens, the corrected gas computation depends on smartAccountVersion?.version but the query key excludes it. A version change won't invalidate cached results.
Proposed fix
- const queryKey = ['gas-estimation', JSON.stringify(clauses), JSON.stringify(tokens), sendingAmount, sendingTokenSymbol];
+ const queryKey = ['gas-estimation', JSON.stringify(clauses), JSON.stringify(tokens), sendingAmount, sendingTokenSymbol, smartAccountVersion?.version];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Only include essential data in query key to prevent unnecessary refetches | |
| const queryKey = ['gas-estimation', JSON.stringify(clauses), JSON.stringify(tokens), sendingAmount, sendingTokenSymbol]; | |
| // Only include essential data in query key to prevent unnecessary refetches | |
| const queryKey = ['gas-estimation', JSON.stringify(clauses), JSON.stringify(tokens), sendingAmount, sendingTokenSymbol, smartAccountVersion?.version]; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/vechain-kit/src/hooks/generic-delegator/useGenericDelegatorFeeEstimation.ts`
around lines 46 - 47, The query key in useGenericDelegatorFeeEstimation is
missing the smartAccountVersion dependency so cache won't invalidate when
smartAccountVersion?.version changes; update the queryKey (the const queryKey
used for gas estimation) to include smartAccountVersion?.version (or the
smartAccountVersion object) alongside clauses, tokens, sendingAmount, and
sendingTokenSymbol so refetches occur when the smart account version updates.
GasFeeTokenSelector item _hover was effectively `'#ffffff12'` for the backgroundColor in every mode (the `textSecondary ? '#ffffff12' : textSecondary` ternary is always truthy since useToken never returns falsy), and used the text-secondary color as the hover border. White- on-white was invisible in light mode, and borrowing a text color for a border was inconsistent with the rest of the kit's hover patterns. GasFeeSummary's outline-style token Button set `_hover.bg = textSecondary` while keeping `color = textSecondary`, so on hover the chip's label disappeared (same colour as its background) — most visible in light mode where textSecondary is a dark grey. Both components now derive a subtle hover overlay from useVeChainKitConfig().darkMode: - dark mode: `rgba(255, 255, 255, 0.08)` (lighten) - light mode: `rgba(0, 0, 0, 0.04)` (darken) GasFeeSummary additionally bumps the button text + border to textPrimary on hover, restoring contrast. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The selected item used `textTertiary` as its background. That token is a text colour — `#718096` (mid-grey) in light mode and rgba(223,223,221, 0.5) (near-white at 50%) in dark mode — which read as a too-strong grey-on-white in light mode and an almost-white-on-dark slab in dark mode. Replace with the same alpha-overlay pattern used for hover, one notch stronger so 'selected' still reads above 'hover': - dark: rgba(255, 255, 255, 0.12) (hover sits at 0.08) - light: rgba(0, 0, 0, 0.06) (hover sits at 0.04) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
main shipped PR #624 which squashed our earlier generic-delegator fix together with the cross-app favicon. The three hook files (useGenericDelegator, useGenericDelegatorFeeEstimation, useEstimateAllTokens) on main are an older snapshot of this branch's work — superseded here by the hoist-estimate-out-of-loop + timeout + always-auto-inject refinements. Take 'ours' for those three files and pick up the new favicon assets (cross-app-connect/src/app/layout.tsx and css-modules.d.ts) from main. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ChooseNameSummaryContent set KIT_PAYS_GAS = true unconditionally, which hid the GasFeeSummary chip, forced hasEnoughGasBalance true, disabled the gas-estimation error banner, and bypassed the loading gate on the Confirm button. But the component routes to useUnsetDomain when isUnsetting=true (see the unsetDomainHook / vetDomainHook / veWorldSubdomainHook switch around line 103), and useUnsetDomain does NOT pass the kit-sponsored delegator URL — the unset path still runs through the user-pays generic delegator. Result: the user got the "VeChain pays" UI but the tx still tried to debit their own VTHO. Compute KIT_PAYS_GAS = !isUnsetting so the unset path falls back to the normal fee-token UI / balance check / estimation-error display. All five downstream gates (shouldEstimateGas, disableConfirmButtonDuringEstimation, hasEnoughBalance, GasFeeSummary render, showGasEstimationError) already read KIT_PAYS_GAS and need no additional changes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
When the user tries to send (almost) their entire balance of a token
that is ALSO the only viable gas token, the fee-estimation iteration
in useGenericDelegatorFeeEstimation rejects every candidate:
- the sending token fails because balance < cost + amount
- the other gas tokens fail because the user doesn't have any
The summary used to surface the resulting error and leave the Confirm
button disabled, forcing the user to go back and manually trim the
amount.
Mirror veworld-mobile's SummaryScreen.tsx co-spend handling: pull the
per-token gas cost via useEstimateAllTokens, and when the iteration
errors out AND the sending token is one of the available gas tokens
with enough balance for the gas alone, set adjustedAmount =
balance - gas * 1.05 (5% safety buffer). The effectiveAmount drives:
- useTransferERC20 / useTransferVET (clauses rebuild with the dropped
amount, so the on-chain transfer matches what we display)
- useGenericDelegatorFeeEstimation's sendingAmount (the iteration
re-runs and now the sending token wins)
- the Amount row in the summary body
- the success-screen description
A blue info-style banner above the Amount row shows
"Amount adjusted from {{original}} to {{adjusted}} {{symbol}} to cover
the transaction fee." so the user understands why the displayed total
shrank between the entry step and the summary.
Swap intentionally NOT included: changing the from-amount in a swap
would invalidate the quote/slippage, so VeWorld's auto-adjust pattern
doesn't fit cleanly there. Left as a follow-up.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two related bugs in the generic-delegator path that surfaced after merging #620 (cross-app whitelabel host):
POST /api/v1/estimate/clauses/{token}simulates the user's raw clauses without theexecuteWithAuthorizationwrapper and without the embedded-wallet typed-data signature, so the returnedtransactionCostis too small (or zero for NFT-heavy clauses where simulation reverts). The kit then built atransferTo<depositAccount>clause for that insufficient amount; co-signing or submission failed and the error was swallowed by the catch atuseGenericDelegator.ts:223-226, surfacing the generic "no gas tokens have sufficient balance or are enabled in Gas Token Preferences" even when the wallet had ample balance.https://mainnet.delegator.vechain.org/api/v1/. The cross-app PR addedfeeDelegation={{ delegatorUrl: process.env.NEXT_PUBLIC_DELEGATOR_URL! }}to the homepage example. That sets the kit'sgenericDelegator=falseand routes the tx through the regular dApp-sponsored fee-delegator path inPrivyWalletProvider.signAndSend, which uses dapp-kit'sProviderInternalBaseWallet({ gasPayer: { gasPayerServiceUrl } })and POSTs the bare tx body to the URL — but that endpoint speaks the generic delegator's API (/sign/transaction/authorized/...), so the POST 404s.What changed
packages/vechain-kit/src/hooks/generic-delegator/useGenericDelegator.tscomputeCorrectedGasTokenCost({ thor, clauses, smartAccountAddress, version, estimationResponse, gasToken }). It callsthor.gas.estimateGaslocally (caller = smart account), adds a fixed wrapper overhead per smart-account version, adds a fixed fee-payer overhead per gas token, applies a 1.1 safety multiplier, and converts back to gas-token units using the delegator's rate (transactionCost / estimatedGas, falling back tovthoPerGasAtSpeed * rate * (1 + serviceFee)).GENERIC_DELEGATOR_GAS_SAFETY_MULTIPLIER,GENERIC_DELEGATOR_FEE_PAYER_OVERHEAD_GAS,GENERIC_DELEGATOR_WRAPPER_OVERHEAD_GAS.sendTransactionUsingGenericDelegatornow derives the transfer amount from the helper instead of trustinggasEstimationResponse.transactionCost. Still one typed-data signature.transactionCost * 2rather than 0.useGenericDelegatorFeeEstimation.ts/useEstimateAllTokens.ts— call the same helper so the fee-estimation UI matches the send path (no more "you have enough VTHO" → throw on click).examples/homepage/.../VechainKitProviderWrapper.tsx— drop thefeeDelegationoverride so the kit falls back togetGenericDelegatorUrl()andgenericDelegator=true. Fixes Bug B.examples/playground/.../VechainKitProviderWrapper.tsx— comment the same override (consistent with homepage).Semantic recap of the
feeDelegationprops (no behavior change)feeDelegation.delegatorUrl→ dApp-sponsored mode, the dApp pays user gas.feeDelegation.genericDelegatorUrl→ override the default generic-delegator endpoint, user still pays.getGenericDelegatorUrl()and uses generic delegator.Things intentionally not changed
useGenericDelegator.ts:223-226still re-throws the generic message. Worth surfacing the realerror.causein a follow-up.feeDelegation.delegatorUrlto the well-known generic-delegator hosts.Test plan
yarn install:all && yarn kit:typecheck && yarn lintfrom repo root.…/api/v1/estimate/clauses/...,…/api/v1/deposit/account,…/api/v1/sign/transaction/authorized/...and the tx mines.safeTransferFromfrom a smart account on mainnet (the original repro). The "no gas tokens have sufficient balance" error must not appear when the wallet has a positive balance; the tx should mine and thetransferToGenericDelegatorClausevalue should be ≥ the actual gas × token-rate.(rawGasResult.totalGas, totalGas, correctedTransactionCost, on-chain gas used)for a couple of real sends; adjustGENERIC_DELEGATOR_WRAPPER_OVERHEAD_GAS/GENERIC_DELEGATOR_FEE_PAYER_OVERHEAD_GASif the spread is consistently too tight or too loose.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Localization