Feat/public fee estimation - #52
Conversation
Exposes the wallet's existing internal fee estimation
(wallet:estimateTransactionFee -> vault.estimateTransactionFee, WASM
auto-calc) as a public dApp-callable provider method, so dApps can show
the required fee without duplicating transaction-building logic.
- Register nock_estimateTransactionFee in PROVIDER_METHODS (defined
locally until @nockbox/iris-sdk >= 0.3.0 is published) and
isProviderMethod()
- New background handler: gated on approved origin + unlocked vault
(read-only, no approval popup, same gating as nock_getWalletInfo);
validates params and returns { fee } in canonical nicks (string)
…mation Previously, omitting fee in nock_sendTransaction failed with 'Missing fee', forcing dApps to ask users for a fee or duplicate wallet/WASM transaction-building logic. Now, when a dApp omits fee: - The background estimates it via vault.estimateTransactionFee before showing the approval popup (estimation failure rejects the request up front with -32603, no dangling pending request) - The approval popup labels the fee as '(estimated)' - On approval, undefined is passed to vault.sendTransactionV2 so WASM auto-calculates the exact fee at build time, eliminating the 'Insufficient fee' late-failure for this path - The dApp response reports the actual fee used (walletTx.fee) Explicitly-provided fees keep the existing behavior verbatim: parsed with allowZero, respected as-is, and echoed back in the response.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 62ba71cf19
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| method === PROVIDER_METHODS.SEND_TRANSACTION || | ||
| method === PROVIDER_METHODS.GET_WALLET_INFO || | ||
| method === PROVIDER_METHODS.SIGN_TX || | ||
| method === PROVIDER_METHODS.ESTIMATE_TRANSACTION_FEE || |
There was a problem hiding this comment.
When a page calls window.nockchain.request({ method: 'nock_estimateTransactionFee', ... }) without an api field, the inpage provider forwards the args unchanged, so resolveSourceApiVersion treats it as legacy v0. Adding this SDK-missing local method to isProviderMethod sends it through @nockbox/iris-sdk@0.2.0's version mapper before the new switch case can run; the adjacent TODO says that SDK version does not define the method yet, so these public requests are rejected during bridging instead of reaching the handler unless the dApp explicitly sets the current API version.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Thanks, but I don't think this holds — I verified it against the actually-installed @nockbox/iris-sdk@0.2.0 mapper.
mapRequest/mapResponse in the bundled compat.js both end in a default: return request/response case, so an unknown method like nock_estimateTransactionFee is not rejected during v0→v1 (or v1→v0) bridging — it passes through untouched and reaches the new switch case. Runtime check against the installed package:
mapRpcRequest({ method: 'nock_estimateTransactionFee', params: { to, amount } }, '0', '1.0.0')
-> { method: 'nock_estimateTransactionFee', params: { to, amount } } // unchanged
mapRpcResponse('nock_estimateTransactionFee', { result: { fee: '504' } }, '1.0.0', '0')
-> { result: { fee: '504' } } // unchanged
So both paths work: a dApp using the SDK provider sends api: '1.0.0' (bridging short-circuits to passthrough at bridgeIncomingProviderPayload), and a dApp calling window.nockchain.request(...) directly without api is treated as v0 and still passes through the mapper's default case. No change needed here.
Generated by Claude Code
Greptile SummaryThis PR adds public fee estimation to the Iris wallet extension, allowing dApps to call
Confidence Score: 4/5Safe to merge with two minor fixes recommended before shipping to users. The core fee-estimation and optional-fee flow is well-structured: early rejection on estimation failure, an explicit feeEstimated flag to avoid stale-fee broadcasts, and a clean fallback to the actual WASM-computed fee in the response. Two issues are worth addressing: the approval screen Total row lacks the estimated marker that the fee row carries, so users see an exact-looking total that may differ slightly from what is actually charged; and the ESTIMATE_TRANSACTION_FEE key name collision between PROVIDER_METHODS and INTERNAL_METHODS silently drops the public method string from the merged RPC_METHODS object, breaking the RPCMethod type union. TransactionApprovalScreen.tsx (missing estimated marker on Total row) and constants.ts (ESTIMATE_TRANSACTION_FEE key collision in RPC_METHODS).
|
| Filename | Overview |
|---|---|
| extension/background/index.ts | Adds optional fee parameter to SEND_TRANSACTION (estimates fee via vault when omitted), adds new ESTIMATE_TRANSACTION_FEE provider case (read-only, gated by approved origin + unlocked vault), and threads feeEstimated through the approval flow so WASM auto-computes the exact fee at broadcast time; logic is sound and error paths are handled. |
| extension/popup/screens/approvals/TransactionApprovalScreen.tsx | Shows "(estimated)" and "~" markers on the fee row when feeEstimated is true; the Total row is missing the same estimated indicator, which could mislead users about the exact amount they are authorising. |
| extension/shared/constants.ts | Extends PROVIDER_METHODS locally with ESTIMATE_TRANSACTION_FEE ahead of SDK 0.3.0; key name collides with INTERNAL_METHODS.ESTIMATE_TRANSACTION_FEE, silently dropping the public method string from the merged RPC_METHODS object and its derived RPCMethod type. |
| extension/shared/types.ts | Adds optional feeEstimated boolean to TransactionRequest to distinguish wallet-estimated fees from explicit dApp-supplied fees; change is additive and backward compatible. |
Comments Outside Diff (2)
-
extension/shared/constants.ts, line 183-186 (link)Key collision drops public method from
RPC_METHODSBoth
PROVIDER_METHODS.ESTIMATE_TRANSACTION_FEE('nock_estimateTransactionFee') andINTERNAL_METHODS.ESTIMATE_TRANSACTION_FEE('wallet:estimateTransactionFee') share the same key name. When both are spread intoRPC_METHODS,INTERNAL_METHODSwins — the public method string'nock_estimateTransactionFee'is silently dropped from the combined object. As a result,RPCMethod(the union derived fromRPC_METHODSvalues) never includes'nock_estimateTransactionFee', so any code that checksmethod as RPCMethodor passes the public method string whereRPCMethodis expected would fail TypeScript's type check. Runtime behaviour is unaffected because all dispatch code reaches the value throughPROVIDER_METHODS.ESTIMATE_TRANSACTION_FEEdirectly, but the type contract is broken.Prompt To Fix With AI
This is a comment left during a code review. Path: extension/shared/constants.ts Line: 183-186 Comment: **Key collision drops public method from `RPC_METHODS`** Both `PROVIDER_METHODS.ESTIMATE_TRANSACTION_FEE` (`'nock_estimateTransactionFee'`) and `INTERNAL_METHODS.ESTIMATE_TRANSACTION_FEE` (`'wallet:estimateTransactionFee'`) share the same key name. When both are spread into `RPC_METHODS`, `INTERNAL_METHODS` wins — the public method string `'nock_estimateTransactionFee'` is silently dropped from the combined object. As a result, `RPCMethod` (the union derived from `RPC_METHODS` values) never includes `'nock_estimateTransactionFee'`, so any code that checks `method as RPCMethod` or passes the public method string where `RPCMethod` is expected would fail TypeScript's type check. Runtime behaviour is unaffected because all dispatch code reaches the value through `PROVIDER_METHODS.ESTIMATE_TRANSACTION_FEE` directly, but the type contract is broken. How can I resolve this? If you propose a fix, please make it concise.
-
extension/popup/screens/approvals/TransactionApprovalScreen.tsx, line 134-142 (link)amount + estimatedFee. A user sees an exact-looking total but may be charged a slightly different amount once WASM auto-calculates the real fee at build time. Adding a~prefix and(estimated)label to the Total row keeps it consistent with the fee row.Prompt To Fix With AI
This is a comment left during a code review. Path: extension/popup/screens/approvals/TransactionApprovalScreen.tsx Line: 134-142 Comment: The "Total" row doesn't carry an estimated indicator even though it's computed as `amount + estimatedFee`. A user sees an exact-looking total but may be charged a slightly different amount once WASM auto-calculates the real fee at build time. Adding a `~` prefix and `(estimated)` label to the Total row keeps it consistent with the fee row. How can I resolve this? If you propose a fix, please make it concise.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
extension/shared/constants.ts:183-186
**Key collision drops public method from `RPC_METHODS`**
Both `PROVIDER_METHODS.ESTIMATE_TRANSACTION_FEE` (`'nock_estimateTransactionFee'`) and `INTERNAL_METHODS.ESTIMATE_TRANSACTION_FEE` (`'wallet:estimateTransactionFee'`) share the same key name. When both are spread into `RPC_METHODS`, `INTERNAL_METHODS` wins — the public method string `'nock_estimateTransactionFee'` is silently dropped from the combined object. As a result, `RPCMethod` (the union derived from `RPC_METHODS` values) never includes `'nock_estimateTransactionFee'`, so any code that checks `method as RPCMethod` or passes the public method string where `RPCMethod` is expected would fail TypeScript's type check. Runtime behaviour is unaffected because all dispatch code reaches the value through `PROVIDER_METHODS.ESTIMATE_TRANSACTION_FEE` directly, but the type contract is broken.
### Issue 2 of 2
extension/popup/screens/approvals/TransactionApprovalScreen.tsx:134-142
The "Total" row doesn't carry an estimated indicator even though it's computed as `amount + estimatedFee`. A user sees an exact-looking total but may be charged a slightly different amount once WASM auto-calculates the real fee at build time. Adding a `~` prefix and `(estimated)` label to the Total row keeps it consistent with the fee row.
```suggestion
<div className="flex justify-between text-sm font-semibold">
<span>Total{isFeeEstimated ? ' (estimated)' : ''}</span>
<div className="text-right">
<div>{formatNock(totalNum / NOCK_TO_NICKS)} NOCK</div>
<div className="text-[10px] font-normal" style={{ color: textMuted }}>
{isFeeEstimated ? '~' : ''}
{formatNick(totalNum)} nicks
</div>
</div>
</div>
```
Reviews (1): Last reviewed commit: "feat: make fee optional in nock_sendTran..." | Re-trigger Greptile
For omitted-fee nock_sendTransaction, approval now passes the wallet's fresh estimate as the fee (matching the popup send flow) instead of undefined. The undefined branch in sendTransactionV2 reserves a 2-NOCK placeholder for note selection, so an account that could afford amount + estimate but not amount + 2 NOCK failed at approval with 'Insufficient available funds'. Passing the concrete estimate sizes note selection to the fee actually shown and charged. Also drops the '~' prefix on the popup fee since the displayed value is now applied verbatim rather than approximated. Addresses review feedback (P2) on PR #52.
2 participants
Summary
nock_estimateTransactionFeeprovider method for approved, unlocked origins.nock_sendTransactioncallers to omitfee.Compatibility and trust boundary
feeretain their previous exact-fee behavior.fee; in that case the displayed fee, total, and resulting balance are marked estimated, with an explicit notice that the final network fee is calculated during building and may differ.Review findings addressed
Validation
f9a62b4: 24 Vitest tests, typecheck, formatting, production build, and production dependency audit pass locally.This standalone PR does not contain the final overlap resolution or bounded retry. Draft PR #55 is the phase-one release artifact and should receive the final human review; do not merge #51/#52 independently for release.