-
Notifications
You must be signed in to change notification settings - Fork 16
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
Add warnings before paratime transactions #236
Conversation
I highly recommend reviewing commit by commit :D |
if (!ownAddresses.includes(this.state.toAddress)) { | ||
return getLanguage("confirmDepositingToParatimeToForeignAccount", "Destination account is not in your wallet! We recommend you always deposit into your own ParaTime account, then transfer from there.") | ||
} | ||
if (ledgerAddresses.includes(this.state.toAddress)) { | ||
return getLanguage("confirmDepositingToParatimeToLedgerAccount", "Destination account was imported from Ledger! Ledger accounts do not support withdrawing from ParaTime.") |
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 being the non-evm side of the branch, it may be more comprehensive to say that Ledger accounts don't support ParaTimes at all? or am I mistaken about that?
the possible risk here is if people think "I'll transfer from Ledger to a non-Ledger account when I want to withdraw" and then find out they can't do that either
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.
Ah, yeah. Not sure of what the best text would be
Ledger accounts do not support withdrawing from ParaTime.
misses some operationsLedger accounts do not support ParaTimes.
I think doesn't sound dangerous enough
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.
Deferred into #238
.map(acc => acc.address) | ||
|
||
if (!ownAddresses.includes(this.state.toAddress)) { | ||
return getLanguage("confirmWithdrawingFromParatimeToForeignAccount", "Destination account is not in your wallet! Some automated systems, e.g., those used for tracking exchange deposits, may be unable to accept funds through ParaTime withdrawals. For better compatibility, cancel, withdraw into your own account, and transfer from there.") |
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.
A note for later, if we ever re-enable displaying the oasis1... address for secp256k1 keys: this check will still prevent people from withdrawing into those addresses, but the message might become a little confusing. We could add an alternative message at that time.
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.
(and message is already slightly inaccurate if user has destination address imported in watch mode
)
Related to #243
Mitigates #190
Probably mitigates #182