Skip to content

Feat/public fee estimation#52

Open
thomas-kroes wants to merge 3 commits into
mainfrom
feat/public-fee-estimation
Open

Feat/public fee estimation#52
thomas-kroes wants to merge 3 commits into
mainfrom
feat/public-fee-estimation

Conversation

@thomas-kroes

Copy link
Copy Markdown
Contributor

No description provided.

claude added 2 commits June 11, 2026 11:05
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 chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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 ||

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Bypass SDK mapping for the local fee method

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 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread extension/background/index.ts Outdated
@greptile-apps

greptile-apps Bot commented Jun 11, 2026

Copy link
Copy Markdown

Greptile Summary

This PR adds public fee estimation to the Iris wallet extension, allowing dApps to call nock_estimateTransactionFee without a user-facing approval popup, and makes the fee parameter optional in nock_sendTransaction so the wallet estimates and auto-calculates it on behalf of the dApp.

  • New provider method (nock_estimateTransactionFee): read-only, gated by approved origin and unlocked vault; calls vault.estimateTransactionFee and returns the fee as a canonical Nicks string.
  • Optional fee in sendTransaction: when fee is omitted the background estimates it upfront (rejecting early on failure), stores feeEstimated: true on the pending request, then passes undefined to WASM at approval time so the exact fee is computed fresh at build time; the actual fee used is reported back to the dApp in the response.
  • UI: the TransactionApprovalScreen now shows "(estimated)" and a "~" prefix on the fee row when the fee was wallet-generated rather than dApp-supplied.

Confidence Score: 4/5

Safe 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).

Important Files Changed

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)

  1. extension/shared/constants.ts, line 183-186 (link)

    P2 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.

    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.
  2. extension/popup/screens/approvals/TransactionApprovalScreen.tsx, line 134-142 (link)

    P2 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.

    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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants