Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: fixing gas estimate on IBC transfers #1607

Merged
merged 2 commits into from
Feb 5, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 5 additions & 7 deletions apps/namadillo/src/App/Common/GasFeeModal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import { useAtomValue } from "jotai";
import { IoClose } from "react-icons/io5";
import { twMerge } from "tailwind-merge";
import { GasConfig } from "types";
import { unknownAsset } from "utils/assets";
import { getDisplayGasFee } from "utils/gas";
import { FiatCurrency } from "./FiatCurrency";
import { TokenCurrency } from "./TokenCurrency";
Expand All @@ -34,6 +33,7 @@ const useBuildGasOption = ({
gasPriceTable: GasPriceTable | undefined;
}) => {
const chainAssetsMap = useAtomValue(chainAssetsMapAtom);

const gasDollarMap =
useAtomValue(
tokenPricesFamily(gasPriceTable?.map((item) => item.token) ?? [])
Expand All @@ -54,10 +54,12 @@ const useBuildGasOption = ({
...override,
};

const displayAmount = getDisplayGasFee(option);
const displayGasFee = getDisplayGasFee(option, chainAssetsMap);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Because of the uosmo address from two channels, we have an intermittent case of showing the osmo fee as zero

But probably something to handle in a different PR

Screenshot 2025-02-05 at 11 58 52

const { totalDisplayAmount: displayAmount, asset } = displayGasFee;
const { symbol } = asset;

const price = gasDollarMap[option.gasToken];
const dollar = price ? price.multipliedBy(displayAmount) : undefined;

const selected =
!gasConfig.gasLimit.isEqualTo(0) &&
option.gasLimit.isEqualTo(gasConfig.gasLimit) &&
Expand All @@ -67,10 +69,6 @@ const useBuildGasOption = ({
const disabled =
gasConfig.gasLimit.isEqualTo(0) || gasConfig.gasPrice.isEqualTo(0);

const asset =
chainAssetsMap[option.gasToken] ?? unknownAsset(option.gasToken);
const symbol = asset.symbol;

return {
option,
selected,
Expand Down
29 changes: 11 additions & 18 deletions apps/namadillo/src/App/Common/TransactionFee.tsx
Original file line number Diff line number Diff line change
@@ -1,28 +1,21 @@
import { chainAssetsMapAtom } from "atoms/chain";
import { useAtomValue } from "jotai";
import { GasConfig } from "types";
import { unknownAsset } from "utils/assets";
import { getDisplayGasFee } from "utils/gas";
import BigNumber from "bignumber.js";
import { TokenCurrency } from "./TokenCurrency";

export const TransactionFee = ({
gasConfig,
}: {
gasConfig: GasConfig;
}): JSX.Element => {
const chainAssetsMap = useAtomValue(chainAssetsMapAtom);

const { gasToken } = gasConfig;
const asset = chainAssetsMap[gasToken] ?? unknownAsset(gasToken);

const amount = getDisplayGasFee(gasConfig);
type TransactionFeeProps = {
displayAmount: BigNumber;
symbol: string;
};

export const TransactionFee = ({
displayAmount,
symbol,
}: TransactionFeeProps): JSX.Element => {
return (
<div className="text-sm">
Transaction fee:{" "}
<TokenCurrency
symbol={asset.symbol}
amount={amount}
symbol={symbol}
amount={displayAmount}
className="font-medium"
/>
</div>
Expand Down
15 changes: 13 additions & 2 deletions apps/namadillo/src/App/Common/TransactionFeeButton.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
import { chainAssetsMapAtom } from "atoms/chain";
import { TransactionFeeProps } from "hooks/useTransactionFee";
import { useState } from "react";
import { useAtomValue } from "jotai";
import { useMemo, useState } from "react";
import { getDisplayGasFee } from "utils/gas";
import { GasFeeModal } from "./GasFeeModal";
import { TransactionFee } from "./TransactionFee";

Expand All @@ -9,14 +12,22 @@ export const TransactionFeeButton = ({
feeProps: TransactionFeeProps;
}): JSX.Element => {
const [modalOpen, setModalOpen] = useState(false);
const chainAssetsMap = useAtomValue(chainAssetsMapAtom);
const gasDisplayAmount = useMemo(() => {
return getDisplayGasFee(feeProps.gasConfig, chainAssetsMap);
}, [feeProps]);

return (
<>
<button
type="button"
className="underline hover:text-yellow transition-all cursor-pointer"
onClick={() => setModalOpen(true)}
>
<TransactionFee gasConfig={feeProps.gasConfig} />
<TransactionFee
displayAmount={gasDisplayAmount.totalDisplayAmount}
symbol={gasDisplayAmount.asset.symbol}
/>
</button>
{modalOpen && (
<GasFeeModal feeProps={feeProps} onClose={() => setModalOpen(false)} />
Expand Down
7 changes: 2 additions & 5 deletions apps/namadillo/src/App/Ibc/ShieldAllPanel.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,7 @@ import {
Stack,
} from "@namada/components";
import svgImg from "App/Assets/ShieldedParty.svg";
import { TransactionFee } from "App/Common/TransactionFee";
import { SelectedWallet } from "App/Transfer/SelectedWallet";
import { getIbcGasConfig } from "integrations/utils";
import { useEffect, useMemo, useState } from "react";
import {
AddressWithAssetAndAmount,
Expand All @@ -32,7 +30,6 @@ type ShieldAllPanelProps = {
};

export const ShieldAllPanel = ({
registry,
wallet,
walletAddress,
isLoading,
Expand Down Expand Up @@ -77,7 +74,7 @@ export const ShieldAllPanel = ({
[selectableAssets]
);

const gasConfig = getIbcGasConfig(registry);
//const gasConfig = getIbcGasConfig(registry);

return (
<ShieldAllContainer>
Expand Down Expand Up @@ -118,7 +115,7 @@ export const ShieldAllPanel = ({
<Stack as="footer" gap={4}>
<footer className="flex justify-between items-center">
<img src={ibcTransferImageBlack} className="w-20" />
{gasConfig && <TransactionFee gasConfig={gasConfig} />}
{/*gasConfig && <TransactionFee gasConfig={gasConfig} />*/}
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we delete this and add a comment like // TODO add transaction fee?

</footer>
<ActionButton
backgroundColor="black"
Expand Down
20 changes: 15 additions & 5 deletions apps/namadillo/src/App/Transfer/TransferDestination.tsx
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
import { Chain } from "@chain-registry/types";
import { Asset, Chain } from "@chain-registry/types";
import { Stack } from "@namada/components";
import { TabSelector } from "App/Common/TabSelector";
import { TransactionFee } from "App/Common/TransactionFee";
import { TransactionFeeButton } from "App/Common/TransactionFeeButton";
import BigNumber from "bignumber.js";
import clsx from "clsx";
import { TransactionFeeProps } from "hooks/useTransactionFee";
import { Address, GasConfig, WalletProvider } from "types";
import { Address, WalletProvider } from "types";
import { ConnectProviderButton } from "./ConnectProviderButton";
import { CustomAddressForm } from "./CustomAddressForm";
import { SelectedChain } from "./SelectedChain";
Expand All @@ -19,7 +20,8 @@ type TransferDestinationProps = {
wallet?: WalletProvider;
walletAddress?: string;
className?: string;
gasConfig?: GasConfig;
gasDisplayAmount?: BigNumber;
gasAsset?: Asset;
feeProps?: TransactionFeeProps;
changeFeeEnabled?: boolean;
customAddressActive?: boolean;
Expand All @@ -40,7 +42,8 @@ export const TransferDestination = ({
isShielded,
isIbcTransfer,
onChangeShielded,
gasConfig,
gasDisplayAmount,
gasAsset,
feeProps,
changeFeeEnabled = true,
customAddressActive,
Expand Down Expand Up @@ -133,7 +136,14 @@ export const TransferDestination = ({
: <div />}
{changeFeeEnabled ?
feeProps && <TransactionFeeButton feeProps={feeProps} />
: gasConfig && <TransactionFee gasConfig={gasConfig} />}
: gasDisplayAmount &&
gasAsset && (
<TransactionFee
displayAmount={gasDisplayAmount}
symbol={gasAsset.symbol}
/>
)
}
</div>
</footer>
</div>
Expand Down
22 changes: 17 additions & 5 deletions apps/namadillo/src/App/Transfer/TransferModule.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,10 @@ import { Chain, Chains } from "@chain-registry/types";
import { ActionButton, Stack } from "@namada/components";
import { mapUndefined } from "@namada/utils";
import { InlineError } from "App/Common/InlineError";
import { chainAssetsMapAtom } from "atoms/chain";
import BigNumber from "bignumber.js";
import { TransactionFeeProps } from "hooks/useTransactionFee";
import { useAtomValue } from "jotai";
import { useMemo, useState } from "react";
import {
Address,
Expand Down Expand Up @@ -115,9 +117,16 @@ export const TransferModule = ({
const [customAddressActive, setCustomAddressActive] = useState(
destination.enableCustomAddress && !destination.availableWallets
);
const chainAssetsMap = useAtomValue(chainAssetsMapAtom);

const [memo, setMemo] = useState<undefined | string>();

const gasConfig = gasConfigProp ?? feeProps?.gasConfig;

const displayGasFee = useMemo(() => {
return gasConfig ? getDisplayGasFee(gasConfig, chainAssetsMap) : undefined;
}, [gasConfig]);

const selectedAsset = mapUndefined(
(address) => source.availableAssets?.[address],
source.selectedAssetAddress
Expand All @@ -132,14 +141,16 @@ export const TransferModule = ({
return undefined;
}

if (!gasConfig || gasConfig.gasToken !== selectedAssetAddress) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The gasConfig.gasToken !== selectedAssetAddress is important because we can pay the gas with a different token and, if this is the case, we shouldn't subtract the gas from the availableAmount

Screenshot 2025-02-05 at 12 15 56

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch!

if (!displayGasFee || !displayGasFee.totalDisplayAmount) {
return availableAmount;
}

const totalFees = getDisplayGasFee(gasConfig);
const amountMinusFees = availableAmount.minus(totalFees);
const amountMinusFees = availableAmount.minus(
displayGasFee.totalDisplayAmount
);

return BigNumber.max(amountMinusFees, 0);
}, [source.selectedAssetAddress, source.availableAmount, gasConfig]);
}, [source.selectedAssetAddress, source.availableAmount, displayGasFee]);

const validationResult = useMemo((): ValidationResult => {
if (!source.wallet) {
Expand Down Expand Up @@ -323,9 +334,10 @@ export const TransferModule = ({
onChangeAddress={destination.onChangeCustomAddress}
memo={memo}
onChangeMemo={setMemo}
gasConfig={gasConfig}
feeProps={feeProps}
changeFeeEnabled={changeFeeEnabled}
gasDisplayAmount={displayGasFee?.totalDisplayAmount}
gasAsset={displayGasFee?.asset}
/>
{isIbcTransfer && requiresIbcChannels && (
<IbcChannels
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import BigNumber from "bignumber.js";
jest.mock("../assets/ibc-transfer-white.png", () => "ibc-transfer-white.png");
jest.mock("../../Common/GasFeeModal", () => null);

Expand All @@ -8,7 +9,7 @@ import {
randomChainMock,
} from "App/Transfer/__mocks__/chains";
import { TransferDestination } from "App/Transfer/TransferDestination";
import BigNumber from "bignumber.js";
import { namadaAsset } from "utils";
import { walletMock } from "../__mocks__/providers";
import { parseChainInfo } from "../common";

Expand Down Expand Up @@ -99,11 +100,8 @@ describe("Component: TransferDestination", () => {
render(
<TransferDestination
changeFeeEnabled={false}
gasConfig={{
gasPrice: BigNumber(0.000001),
gasLimit: BigNumber(2),
gasToken: "tnam1",
}}
gasDisplayAmount={new BigNumber("0.000002")}
gasAsset={namadaAsset()}
/>
);
const transactionFee = screen.getByText("Transaction fee:");
Expand Down
6 changes: 5 additions & 1 deletion apps/namadillo/src/App/Transfer/common.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,5 +23,9 @@ export const parseChainInfo = (
};

export const isShieldedAddress = (address: string): boolean => {
return address.startsWith("znam"); // TODO: integrate with registry
return address.startsWith("znam");
};

export const isTransparentAddress = (address: string): boolean => {
return address.startsWith("tnam");
};
21 changes: 15 additions & 6 deletions apps/namadillo/src/integrations/utils.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
import { Asset, Chain } from "@chain-registry/types";
import { Bech32Config, ChainInfo, Currency } from "@keplr-wallet/types";
import tokenImage from "App/Common/assets/token.svg";
import { getRestApiAddressByIndex, getRpcByIndex } from "atoms/integrations";
import {
getKnownChains,
getRestApiAddressByIndex,
getRpcByIndex,
} from "atoms/integrations";
import BigNumber from "bignumber.js";
import { ChainId, ChainRegistryEntry, GasConfig } from "types";

Expand All @@ -27,11 +31,16 @@ export const findRegistryByChainId = (
return undefined;
};

export const findAssetByDenom = (
registry: ChainRegistryEntry,
denom: string
): Asset | undefined => {
return registry.assets.assets.find((asset) => asset.base === denom);
export const findAssetByDenom = (denom: string): Asset | undefined => {
const chainRegistry = getKnownChains();
if (!chainRegistry) return undefined;

for (const registry of chainRegistry) {
const asset = registry.assets.assets.find((asset) => asset.base === denom);
if (asset) return asset;
}

return undefined;
};

export const getAssetImageUrl = (asset?: Asset): string => {
Expand Down
5 changes: 5 additions & 0 deletions apps/namadillo/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,11 @@ export type GasConfig = {
gasToken: GasToken;
};

export type GasConfigToDisplay = {
totalDisplayAmount: BigNumber;
asset: Asset;
};

export type TxGas = Record<Address, GasLimit>;

export type GasTable = Record<TxKind, TxGas>;
Expand Down
39 changes: 34 additions & 5 deletions apps/namadillo/src/utils/gas.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,37 @@
import { Asset } from "@chain-registry/types";
import { isTransparentAddress } from "App/Transfer/common";
import BigNumber from "bignumber.js";
import { GasConfig } from "types";
import { findAssetByDenom } from "integrations/utils";
import { Address, GasConfig, GasConfigToDisplay } from "types";
import { isNamadaAsset, toDisplayAmount } from "utils";
import { unknownAsset } from "./assets";

export const getDisplayGasFee = (gasConfig: GasConfig): BigNumber => {
return BigNumber(gasConfig.gasPrice)
.multipliedBy(gasConfig.gasLimit)
.decimalPlaces(6);
export const calculateGasFee = (gasConfig: GasConfig): BigNumber => {
return BigNumber(gasConfig.gasPrice).multipliedBy(gasConfig.gasLimit);
};

export const getDisplayGasFee = (
gasConfig: GasConfig,
chainAssetsMap?: Record<Address, Asset>
): GasConfigToDisplay => {
const { gasToken } = gasConfig;
let asset: Asset;

if (isTransparentAddress(gasToken) && chainAssetsMap) {
// The gasConfig token might be the address of the token on Namada chain
asset = chainAssetsMap[gasToken] ?? unknownAsset(gasToken);
} else {
// However, if the gasConfig contains a token used by Keplr, it could be the asset
// denomination unit, like "uosmo"
asset = findAssetByDenom(gasToken) ?? unknownAsset(gasToken);
}

const totalDisplayAmount = calculateGasFee(gasConfig);
return {
totalDisplayAmount:
isNamadaAsset(asset) ? totalDisplayAmount : (
toDisplayAmount(asset, totalDisplayAmount).decimalPlaces(6)
),
asset,
};
};