-
Notifications
You must be signed in to change notification settings - Fork 120
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
Namadillo: IBC withdrawal improvements #1446
Conversation
useEffect(() => { | ||
if (transaction?.hash) { | ||
const tx = findByHash(transaction.hash); | ||
tx && setTransaction(tx); |
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.
Thinking loud, how about if we create a new route/page that handle this tx progress? Then, this useEffect executes an route redirect
Sometimes I had a not so good UX when I'm on the progress page, then I click on menu Ibc button to execute a second Ibc and it doesn't navigates. Same if I click on the back button of the browser
Maybe we should just go directly to the History
single page, like /transaction/{hash}
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.
We already have this page. However, if the transaction fails for some reason, we're returning to the previous screen, so the user can change the params. I think it would be too complicate to handle this case if we're redirecting the users to a new page directly. Any ideas?
@@ -204,7 +204,7 @@ export const MaspShield: React.FC = () => { | |||
/> | |||
</motion.div> | |||
)} | |||
{currentStep > 0 && ( | |||
{currentStep > 0 && transaction?.currentStep && ( |
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.
Should we rename this to currentStepIndex as well? Just to be equal do MaspUnshiled?
destinationAddress: Address, | ||
displayAmount: BigNumber | ||
): Promise<TransferTransactionData> => { | ||
assertValid(); |
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.
This implementation is breaking the type check and requiring you to add the !
on the variables below.
To avoid this issue, we could update it to be like:
invariant(sourceAddress, "Source address is not defined")
invariant(selectedAsset, "No asset is selected")
invariant(registry, "Invalid chain")
invariant(sourceChannel, "Invalid IBC source channel")
invariant(shielded && !destinationChannel, "Invalid IBC destination channel")
invariant(gasConfig, "No transaction fee is set")
I understand it could be too verbose, but it's quite often that we catch an border case error because of these !
Another alternative is creating a type that holds all params as required and build this object with some type check on another function, like buildIbcTxParams: (sourceAddress?: string .... ) => IbcTxParams
This is not the final state of IBC withdrawals, we still need to treat errors in a better way.
This PR contains the integration of IBC withdrawals with the indexer and several improvements in the workflow.
It also refactors IBC transfers.
🎄 🎅