-
Notifications
You must be signed in to change notification settings - Fork 122
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
Changes from all commits
f6d523e
ec2b8e2
e20dc26
e8d5ec1
682b25b
466312e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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(""); | ||
const rpcUrl = useAtomValue(rpcUrlAtom); | ||
const chainParameters = useAtomValue(chainParametersAtom); | ||
const defaultAccounts = useAtomValue(allDefaultAccountsAtom); | ||
|
@@ -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 | ||
|
@@ -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 => { | ||
|
@@ -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"); | ||
|
@@ -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 | ||
|
@@ -154,6 +155,7 @@ export const MaspShield: React.FC = () => { | |
isSubmitting={isPerformingTransfer} | ||
errorMessage={generalErrorMessage} | ||
onSubmitTransfer={onSubmitTransfer} | ||
submittingText={isPerformingTransfer ? currentStep : "Shield"} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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", | ||
}} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -80,10 +80,6 @@ const IncrementBonding = (): JSX.Element => { | |
</> | ||
), | ||
}), | ||
parseErrorTxNotification: () => ({ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
title: "Staking transaction failed", | ||
description: "", | ||
}), | ||
onBroadcasted: () => { | ||
onCloseModal(); | ||
}, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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"), | ||
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
}; | ||
}, | ||
}; | ||
}), | ||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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