feat: improving masp transactions feedback for users and other gas estimation fixes #1594
Conversation
pedrorezende
requested review from
euharrison,
jurevans and
mateuszjasiuk
and removed request for
euharrison and
jurevans
| @@ -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.
We could talk with Chris later to improve this change visually, maybe with animations 🌟
| const [displayAmount, setDisplayAmount] = useState<BigNumber | undefined>(); | ||
| const [generalErrorMessage, setGeneralErrorMessage] = useState(""); | ||
|
|
||
| const [currentStep, setCurrentStep] = useState(""); |
There was a problem hiding this comment.
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.
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
| @@ -80,10 +80,6 @@ const IncrementBonding = (): JSX.Element => { | |||
| </> | |||
| ), | |||
| }), | |||
| parseErrorTxNotification: () => ({ | |||
There was a problem hiding this comment.
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
| // 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.
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.
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
apps/namadillo/src/lib/query.ts
Outdated
| if (!publicKeyRevealed) { | ||
| const revealPkTx = await tx.buildRevealPk(wrapperTxProps); | ||
| txs.push(revealPkTx); | ||
| if (!skipRevealPk) { |
There was a problem hiding this comment.
just to avoid "not-not", how about renaming this to revealPk?: boolean = true and if (revealPk)?
| | UseTransactionOutput<TransparentTransferMsgValue> | ||
| | UseTransactionOutput<ShieldedTransferMsgValue> | ||
| | UseTransactionOutput<ShieldingTransferMsgValue> | ||
| | UseTransactionOutput<UnshieldingTransferMsgValue> |
pedrorezende
force-pushed
the
feat/multiple-masp-stages
branch
from
d576050 to
682b25b
Compare
This PR refactors all the transactions logic separating the build step from the signing step. With this we will be able to improve user experience while waiting for long steps, like the generation of masp params. I've only implemented multiple status on Shielding because we're going to change a few things on the UI in the next PRs.
It also creates a quick workaround to increase gas fees while we improve the indexer api for estimate. The reason behind it is that many transactions were frequently failing because of gas (ex: unstake, re-delegate, etc)