close
Skip to content

Improve fees display on send#2649

Open
leofelix077 wants to merge 46 commits intomasterfrom
feature/improve-resource-fee-display-on-send
Open

Improve fees display on send#2649
leofelix077 wants to merge 46 commits intomasterfrom
feature/improve-resource-fee-display-on-send

Conversation

@leofelix077
Copy link
Copy Markdown
Collaborator

@leofelix077 leofelix077 commented Mar 12, 2026

closes #2475

Screen.Recording.2026-03-13.at.11.42.36.mov
Screen.Recording.2026-03-13.at.11.43.27.mov

@leofelix077 leofelix077 self-assigned this Mar 12, 2026
@leofelix077 leofelix077 added wip not for merging yet don't review yet wip / tests in progress / missing videos or screenshots / pending self code-review labels Mar 12, 2026
Copilot AI review requested due to automatic review settings March 12, 2026 20:33
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR improves the resource fee display on the send flow by adding a detailed fee breakdown pane to the review transaction screen, showing inclusion fee and resource fee separately for Soroban transactions.

Changes:

  • Added a new "Fee breakdown" pane in ReviewTransaction showing inclusion fee, resource fee, and total fee with contextual descriptions for classic vs Soroban transactions
  • Propagated inclusionFee and resourceFee from simulation results through both send and collectible flows
  • Added "Calculating..." loading state for fees and a loading indicator on the continue button during simulation

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
extension/src/popup/components/InternalTransaction/ReviewTransaction/index.tsx New fees breakdown pane with fee detail rows and info button
extension/src/popup/components/InternalTransaction/ReviewTransaction/styles.scss Styles for the fees breakdown pane and info button
extension/src/popup/components/send/SendAmount/index.tsx Show "Calculating..." during simulation, loading state on button
extension/src/popup/components/send/SendAmount/hooks/useSimulateTxData.tsx Pass inclusionFee and resourceFee from simulation
extension/src/popup/components/sendCollectible/SelectedCollectible/hooks/useSimulateTxData.ts Pass inclusionFee and resourceFee from simulation
extension/src/popup/locales/en/translation.json New i18n keys for fee breakdown UI
extension/src/popup/locales/pt/translation.json Portuguese translations for new keys (with one regression)
@shared/api/internal.ts Added network_url to simulate-tx request body
extension/e2e-tests/reviewTxFees.test.ts E2E tests for fee breakdown in token, classic, and collectible sends

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread extension/src/popup/locales/pt/translation.json Outdated
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@leofelix077 leofelix077 changed the title Feature/improve resource fee display on send Improve resource fee display on send Mar 13, 2026
….com:stellar/freighter into feature/improve-resource-fee-display-on-send
@leofelix077 leofelix077 changed the title Improve resource fee display on send Improve fees display on send Mar 13, 2026
@leofelix077 leofelix077 requested a review from Copilot March 13, 2026 15:50
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread extension/src/popup/components/InternalTransaction/FeesPane/index.tsx Outdated
Comment thread extension/src/popup/components/InternalTransaction/FeesPane/index.tsx Outdated
@leofelix077 leofelix077 added enhancement New feature or request and removed wip not for merging yet don't review yet wip / tests in progress / missing videos or screenshots / pending self code-review labels Mar 13, 2026
@sdfcharles
Copy link
Copy Markdown
Contributor

sdfcharles commented Mar 13, 2026

@leofelix077 looks nice! few notes:

CleanShot 2026-03-13 at 11 06 29
  1. should this be Inclusion fee?
CleanShot 2026-03-13 at 11 07 24
  1. lets match the top/bottom padding here
CleanShot 2026-03-13 at 13 18 43
  1. lets move the copy button next to the text
CleanShot 2026-03-13 at 13 20 47
  1. i like the idea of the buttons here but lets drop them for now for consistency

leofelix077 and others added 3 commits March 16, 2026 09:30
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Comment thread extension/src/popup/components/send/SendAmount/index.tsx Outdated
Comment thread extension/src/popup/components/send/SendAmount/index.tsx Outdated
Comment thread extension/src/popup/components/InternalTransaction/FeesPane/index.tsx Outdated
Comment thread extension/src/popup/components/send/SendAmount/index.tsx
Comment thread extension/src/popup/components/send/SendAmount/index.tsx
Comment thread extension/src/popup/components/send/SendAmount/index.tsx
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread extension/src/popup/components/send/SendAmount/index.tsx Outdated
Comment thread extension/src/popup/components/send/SendAmount/index.tsx Outdated
Copy link
Copy Markdown
Contributor

@CassioMG CassioMG left a comment

Choose a reason for hiding this comment

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

Found one last issue that would be worth addressing on this PR before merging it, specially since same bug is being addressed on the mobile PR so we keep parity.

memo,
params,
networkDetails,
transactionFee,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is an existing bug from before this PR but I think is worth addressing since it's related to fee handling (this is already fixed on mobile's PR): it looks like the transactionFee is being passed here but it's being discarded inside simulateTokenTransfer which means it's not being passed to the backend so the backend is always using the default fee of 100 stroops. This means the inclusion fee users are setting on transaction settings are not being effectively used.

Below is a more in-depth analysis from AI:

The user-selected inclusion fee is dropped before reaching the backend on the indexer/mainnet path.

transactionFee is correctly threaded into simulateTokenTransfer({...transactionFee}) here, but in @shared/api/internal.ts the non-custom-network branch (around line 2186) builds requestBody without including any fee field:

const requestBody = {
  address,
  pub_key: publicKey,
  memo: memo || "",
  params,
  network_passphrase: networkDetails.networkPassphrase,
  // fee is NOT included
};

The freighter-backend /simulate-token-transfer handler accepts an optional fee and falls back to BASE_FEE when absent:

const _fee = fee || Sdk.BASE_FEE;
const builder = new Sdk.TransactionBuilder(sourceAccount, { fee: _fee, ... });
// ... assembleTransaction → returned preparedTransaction.fee = _fee + minResourceFee

Since the client submits the returned preparedTransaction XDR verbatim, the on-chain inclusion bid is always BASE_FEE (100 stroops) regardless of what the user picks in the slider. The new FeesPane shows the user's selected value as the inclusion fee, but the chain sees BASE_FEE.

Impact: invisible during normal traffic (BASE_FEE suffices), but during surge pricing — the case the slider exists for — bumping the fee has no effect; the tx loses inclusion auctions and times out.

Fix: add one line to the request body in internal.ts:

const requestBody = {
  address,
  pub_key: publicKey,
  memo: memo || "",
  fee: xlmToStroop(transactionFee).toFixed(),
  params,
  network_passphrase: networkDetails.networkPassphrase,
};

The mobile companion PR (stellar/freighter-mobile#774) made this same change (fee: xlmToStroop(fee).toString()), and the freighter-backend already supports the field.

@piyalbasu
Copy link
Copy Markdown
Contributor

Code review

Found 2 issues:

  1. The send-amount-fee-display "Calculating..." gate only fires for RequestState.LOADING, dropping the RequestState.ERROR arm an earlier branch commit (c0973c98, "sync state with extension and harden validation for simulation errors") had explicitly added. On a Soroban simulation failure the label silently reverts to the prior-simulation fee value while the Continue button is disabled with no other affordance, leaving users to wonder why they can't proceed.

</span>
<span data-testid="send-amount-fee-display">
{(isToken || isCollectible) &&
simulationState.state === RequestState.LOADING
? t("Calculating...")
: inputType === "crypto"
? `${fee} ${t("XLM")}`
: recommendedFeeUsd}
</span>
</div>

  1. goToChooseAssetAction clears manualTransactionFee and hasManuallySetFeeRef, but unlike the sibling goBackAction (which dispatches both saveTransactionFee("") and saveManualTransactionFee(null)) it leaves transactionFee in Redux. The block comment says "Clear it here before navigating so post-remount the fee is derived freshly from the new asset's simulation," but fee = transactionFee || recommendedFee will still pick up the prior asset's saved total on remount until the auto-simulation useEffect runs, contradicting the stated invariant.

const goBackAction = () => {
dispatch(saveAsset("native"));
dispatch(saveIsToken(false));
dispatch(saveAmount("0"));
dispatch(saveAmountUsd("0.00"));
// Clear any manually-saved fee so the next send session always starts from
// the simulated base fee rather than a stale override.
dispatch(saveTransactionFee(""));
dispatch(saveManualTransactionFee(null));
goBack();
if (isCollectible) {
goToChooseAssetAction();
}
};
const goToChooseAssetAction = () => {
// Changing the asset may switch between Soroban and classic (or a different
// token), so any manually-saved fee from the prior asset should not carry
// over. Clear it here before navigating so post-remount the fee is derived
// freshly from the new asset's simulation.
dispatch(saveManualTransactionFee(null));
hasManuallySetFeeRef.current = null;
goToChooseAsset();
};

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

@piyalbasu
Copy link
Copy Markdown
Contributor

Also, I noticed a few UI bugs here:

1). If you update the fee and then click the i bubble, the modal shows the old fee
2) The default button next to the fee appears to have stopped working
3) i'm able to save with an empty inclusion fee

Screen.Recording.2026-04-28.at.5.12.00.PM.mov

@leofelix077
Copy link
Copy Markdown
Collaborator Author

@piyalbasu @CassioMG thanks for checking!

I updated it now the UI + verifying that it's carrying forward the fee to the simulation

Screen.Recording.2026-04-29.at.12.57.28.mov
Screenshot 2026-04-29 at 13 01 04

Copy link
Copy Markdown
Contributor

@CassioMG CassioMG left a comment

Choose a reason for hiding this comment

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

Re-reviewed at e9e5a7a. The previous concern about fee being dropped before reaching the backend is resolved — the new fee: xlmToStroop(transactionFee).toFixed() line in @shared/api/internal.ts and the test in @shared/api/__tests__/internal.test.ts confirm the wiring end-to-end.

Two Important findings, and a Small finding remain — see inline comments.

Comment thread extension/src/popup/components/swap/SwapAmount/index.tsx Outdated
Comment thread extension/src/popup/components/send/SendAmount/index.tsx Outdated
Comment thread @shared/api/internal.ts
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Important — this hook misses the first-mount fee fix that was applied to the send hook.

Lines 109-110 (outside this PR's diff hunks, hence the file-level comment) still use the old pattern:

const currentTransactionFee =
  currentTransactionData.transactionFee || transactionFee;

The new getCurrentTransactionFee helper added in send/SendAmount/hooks/useSimulateTxData.tsx:123-132 (with the BASE_FEE fallback) was the fix for the first-mount-fee-zero issue I flagged at #2649 (comment). The send hook now uses it; this hook does not.

Why it matters here too: on first-mount of a collectible send, currentTransactionData.transactionFee = "" and transactionFee = "", so currentTransactionFee = "". Then simulateTx (line 124) receives recommendedFee: ""simulateSendCollectible is called with transactionFee: ""xlmToStroop("").toFixed() === "0" is set as the TransactionBuilder fee (@shared/api/internal.ts:2080). stellar-sdk's TransactionBuilder rejects fees below BASE_FEE at submit time.

Why it's mostly latent today: like the send case, handleContinue paths typically reset transactionFee before re-simulation, so the user-submitted XDR is from a re-sim with a proper fee. But the first sim still wastes a build cycle and produces an invalid XDR, and any future code path that submits the first-sim output silently breaks.

Suggested fix: export getCurrentTransactionFee from a shared location (e.g. popup/helpers/fees.ts or popup/helpers/soroban.ts) and use it here too:

import { getCurrentTransactionFee } from "popup/helpers/fees";

const currentTransactionFee = getCurrentTransactionFee({
  currentTransactionFee: currentTransactionData.transactionFee,
  fallbackTransactionFee: transactionFee,
});

Keeping the helper inside the send/SendAmount hook directory while the collectible hook needs it would force this hook to reach across component boundaries, which is a smell — the helper lives in the wrong place even before the parity fix.

import { NetworkDetails } from "@shared/constants/stellar";
import { stroopToXlm } from "helpers/stellar";
import { getBaseAccount } from "popup/helpers/account";
import { getCurrentTransactionFee } from "popup/components/send/SendAmount/hooks/useSimulateTxData";
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should we extract it to a helper as suggested here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve minResourceFee display in Send flow

5 participants