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

feat: improving masp transactions feedback for users and other gas estimation fixes #1594

Merged
merged 6 commits into from
Feb 3, 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
1 change: 0 additions & 1 deletion apps/namadillo/src/App/Common/TransactionFeeButton.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ export const TransactionFeeButton = ({
feeProps: TransactionFeeProps;
}): JSX.Element => {
const [modalOpen, setModalOpen] = useState(false);

return (
<>
<button
Expand Down
10 changes: 6 additions & 4 deletions apps/namadillo/src/App/Masp/MaspShield.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,10 @@ import { Address, TransferTransactionData } from "types";
export const MaspShield: React.FC = () => {
const navigate = useNavigate();
const [searchParams, setSearchParams] = useSearchParams();
const { storeTransaction } = useTransactionActions();
const [displayAmount, setDisplayAmount] = useState<BigNumber | undefined>();
const [generalErrorMessage, setGeneralErrorMessage] = useState("");

const [currentStep, setCurrentStep] = useState("");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you apply this on MaspUnshield and NamadaTransfer as well? Or is this part of a future PR?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hello! This is going to be part of a PR I'm working now. We're going to change the whole interface after submitting the transaction, that's why I didn't apply this change to other pages

const rpcUrl = useAtomValue(rpcUrlAtom);
const chainParameters = useAtomValue(chainParametersAtom);
const defaultAccounts = useAtomValue(allDefaultAccountsAtom);
Expand All @@ -37,8 +38,6 @@ export const MaspShield: React.FC = () => {
namadaTransparentAssetsAtom
);

const { storeTransaction } = useTransactionActions();

const chainId = chainParameters.data?.chainId;
const sourceAddress = defaultAccounts.data?.find(
(account) => account.type !== AccountType.ShieldedKeys
Expand All @@ -61,6 +60,7 @@ export const MaspShield: React.FC = () => {
target: destinationAddress ?? "",
token: selectedAsset?.originalAddress ?? "",
displayAmount: displayAmount ?? new BigNumber(0),
onUpdateStatus: setCurrentStep,
});

const onChangeSelectedAsset = (address?: Address): void => {
Expand Down Expand Up @@ -88,6 +88,7 @@ export const MaspShield: React.FC = () => {
}: OnSubmitTransferParams): Promise<void> => {
try {
setGeneralErrorMessage("");
setCurrentStep("");

invariant(sourceAddress, "Source address is not defined");
invariant(chainId, "Chain ID is undefined");
Expand Down Expand Up @@ -120,7 +121,7 @@ export const MaspShield: React.FC = () => {
};

return (
<Panel className="relative min-h-[600px]">
<Panel className="relative min-h-[600px] flex-1">
<header className="flex flex-col items-center text-center mb-3 gap-6">
<h1 className="mt-6 text-lg text-yellow">Shield</h1>
<NamadaTransferTopHeader
Expand Down Expand Up @@ -154,6 +155,7 @@ export const MaspShield: React.FC = () => {
isSubmitting={isPerformingTransfer}
errorMessage={generalErrorMessage}
onSubmitTransfer={onSubmitTransfer}
submittingText={isPerformingTransfer ? currentStep : "Shield"}
Copy link
Collaborator

Choose a reason for hiding this comment

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

We could talk with Chris later to improve this change visually, maybe with animations 🌟

buttonTextErrors={{
NoAmount: "Define an amount to shield",
}}
Expand Down
4 changes: 0 additions & 4 deletions apps/namadillo/src/App/Staking/IncrementBonding.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -80,10 +80,6 @@ const IncrementBonding = (): JSX.Element => {
</>
),
}),
parseErrorTxNotification: () => ({
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we remove this, the notification failed doesn't appear. I tested this trying to stake with a custom small amoutn of gas (0.01 NAM) and you can observe the notification there forever

Screenshot 2025-01-31 at 14 06 47

title: "Staking transaction failed",
description: "",
}),
onBroadcasted: () => {
onCloseModal();
},
Expand Down
4 changes: 0 additions & 4 deletions apps/namadillo/src/App/Staking/ReDelegate.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -65,10 +65,6 @@ export const ReDelegate = (): JSX.Element => {
title: "Staking redelegation in progress",
description: <>Your redelegation transaction is being processed</>,
}),
parseErrorTxNotification: () => ({
title: "Staking redelegation failed",
description: "",
}),
onBroadcasted: () => {
onCloseModal();
},
Expand Down
4 changes: 0 additions & 4 deletions apps/namadillo/src/App/Staking/Unstake.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -67,10 +67,6 @@ export const Unstake = (): JSX.Element => {
</>
),
}),
parseErrorTxNotification: () => ({
title: "Unstake transaction failed",
description: "",
}),
onBroadcasted: () => {
onCloseModal();
},
Expand Down
5 changes: 1 addition & 4 deletions apps/namadillo/src/App/Transfer/TransferModule.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -107,8 +107,6 @@ export const TransferModule = ({
errorMessage,
buttonTextErrors = {},
}: TransferModuleProps): JSX.Element => {
const gasConfig = gasConfigProp ?? feeProps?.gasConfig;

const [walletSelectorModalOpen, setWalletSelectorModalOpen] = useState(false);
const [sourceChainModalOpen, setSourceChainModalOpen] = useState(false);
const [destinationChainModalOpen, setDestinationChainModalOpen] =
Expand All @@ -117,17 +115,16 @@ export const TransferModule = ({
const [customAddressActive, setCustomAddressActive] = useState(
destination.enableCustomAddress && !destination.availableWallets
);

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

const gasConfig = gasConfigProp ?? feeProps?.gasConfig;
const selectedAsset = mapUndefined(
(address) => source.availableAssets?.[address],
source.selectedAssetAddress
);

const availableAmountMinusFees = useMemo(() => {
const { selectedAssetAddress, availableAmount } = source;

if (
typeof selectedAssetAddress === "undefined" ||
typeof availableAmount === "undefined"
Expand Down
15 changes: 14 additions & 1 deletion apps/namadillo/src/atoms/fees/atoms.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,8 @@ export const gasEstimateFamily = atomFamily(
}
const counter = (kind: TxKind): number | undefined =>
txKinds.filter((i) => i === kind).length || undefined;
return fetchGasEstimate(api, [

const gasEstimate = await fetchGasEstimate(api, [
counter("Bond"),
counter("ClaimRewards"),
counter("Unbond"),
Expand All @@ -51,6 +52,18 @@ export const gasEstimateFamily = atomFamily(
counter("RevealPk"),
counter("Redelegate"),
]);

// TODO: we need to improve this estimate API. Currently, gasEstimate.min returns
// the minimum gas limit ever used for a TX, and avg is failing in most of the transactions.
// So we estabilish the average value between gasEstimate.avg and gasEstimate.max as the min value.
// The other values derive from this one.
const newMin = Math.ceil((gasEstimate.avg + gasEstimate.max) / 2);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I understand the reason, but I'm afraid of this math creates an infinite loop of gas growing infinitely. We need to discuss this with other folks, but I'm good for now on campfire

For me, this endpoint should be the return value of a dry-run, then we apply a const like the 1.25 and 1.5 you did

Copy link
Collaborator Author

@pedrorezende pedrorezende Feb 3, 2025

Choose a reason for hiding this comment

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

We can't dry run because we would need to sign. I've talked to @Fraccaman about this, and this was the best shot we have for now. Don't think it will change gas on indexer infinitely, since it will be a security issue. I think indexer takes in consideration the gas effectively used, not the amount users define

return {
min: newMin,
avg: Math.ceil(newMin * 1.25),
max: Math.ceil(newMin * 1.5),
totalEstimates: gasEstimate.totalEstimates,
};
},
};
}),
Expand Down
4 changes: 2 additions & 2 deletions apps/namadillo/src/atoms/integrations/atoms.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import invariant from "invariant";
import { atom } from "jotai";
import { atomWithMutation, atomWithQuery } from "jotai-tanstack-query";
import { atomFamily, atomWithStorage } from "jotai/utils";
import { TransactionPair } from "lib/query";
import { EncodedTxData } from "lib/query";
import {
AddressWithAssetAndAmountMap,
BuildTxAtomParams,
Expand Down Expand Up @@ -164,7 +164,7 @@ export const createIbcTxAtom = atomWithMutation((get) => {
account,
gasConfig,
}: BuildTxAtomParams<IbcTransferMsgValue>): Promise<
TransactionPair<IbcTransferProps> | undefined
EncodedTxData<IbcTransferProps> | undefined
> => {
if (typeof account === "undefined") {
throw new Error("no account");
Expand Down
17 changes: 7 additions & 10 deletions apps/namadillo/src/atoms/integrations/functions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import * as elysTestnet from "chain-registry/testnet/elystestnet";
import * as osmosisTestnet from "chain-registry/testnet/osmosistestnet";
import * as stargazeTestnet from "chain-registry/testnet/stargazetestnet";
import { DenomTrace } from "cosmjs-types/ibc/applications/transfer/v1/transfer";
import { TransactionPair, buildTxPair } from "lib/query";
import { EncodedTxData, buildTx } from "lib/query";
import {
Address,
AddressWithAssetAndAmount,
Expand Down Expand Up @@ -365,9 +365,8 @@ export const createIbcTx = async (
gasConfig: GasConfig,
chain: ChainSettings,
memo?: string
): Promise<TransactionPair<IbcTransferProps>> => {
const { tx } = await getSdkInstance();

): Promise<EncodedTxData<IbcTransferProps>> => {
const sdk = await getSdkInstance();
const ibcTransferProps = {
source: account.address,
receiver: destinationAddress,
Expand All @@ -377,17 +376,15 @@ export const createIbcTx = async (
channelId,
memo,
};

const transactionPair = await buildTxPair(
return await buildTx(
sdk,
account,
gasConfig,
chain,
[ibcTransferProps],
tx.buildIbcTransfer,
account.address
sdk.tx.buildIbcTransfer,
memo
);

return transactionPair;
};

export const namadaLocal = (chainId: string): Chain => {
Expand Down
14 changes: 6 additions & 8 deletions apps/namadillo/src/atoms/proposals/functions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import { assertNever, mapUndefined } from "@namada/utils";
import BigNumber from "bignumber.js";
import * as E from "fp-ts/Either";
import * as t from "io-ts";
import { TransactionPair, buildTxPair } from "lib/query";
import { TransactionPair, buildTx, signEncodedTx } from "lib/query";
import { GasConfig } from "types";

import { fromHex } from "@cosmjs/encoding";
Expand Down Expand Up @@ -373,23 +373,21 @@ export const createVoteProposalTx = async (
chain: ChainSettings
): Promise<TransactionPair<VoteProposalProps>> => {
try {
const { tx } = await getSdkInstance();

const sdk = await getSdkInstance();
const voteProposalProps = {
signer: account.address,
proposalId,
vote,
};

const transactionPair = await buildTxPair(
const encodedTx = await buildTx(
sdk,
account,
gasConfig,
chain,
[voteProposalProps],
tx.buildVoteProposal,
account.address
sdk.tx.buildVoteProposal
);
return transactionPair;
return await signEncodedTx(encodedTx, account.address);
} catch (err) {
console.error(err);
throw err;
Expand Down
Loading