fix: fixing when the dangerous warning is presented [PERA-3786]#198
fix: fixing when the dangerous warning is presented [PERA-3786]#198
Conversation
wjbeau
left a comment
wjbeau
left a comment
There was a problem hiding this comment.
I think this PR duplicates some existing logic. In useSigningRequestAnalysis we have some of this same functionality for the same purpose. Feels like we should unify this, and I guess this overlaps wiht the work @yasincaliskan is doing too?
|
Also we should apply the same logic to rekeys (i.e. only consider them a warning if they are rekeying your account), though I think the useSigningRequestAnalysis has a flaw there in that we should probably just exclude watch accounts and not non-signing accounts... |
|
@wjbeau Ok, I'll hold of with work on this PR until @yasincaliskan is done with his PR so I can check that again in a better state of the codebase. |
yasincaliskan
left a comment
yasincaliskan
left a comment
There was a problem hiding this comment.
I think it would be good to rebase this branch to main to see what useSigningPipeline does. Because signing refactor already handles a lot of this warning logic.
| const isSenderUserControlled = useMemo(() => { | ||
| if (!transaction.sender) return false | ||
| const senderAccount = accounts.find( | ||
| a => a.address === transaction.sender, | ||
| ) | ||
| return senderAccount | ||
| ? canSignWithAccount(senderAccount, accounts) | ||
| : false | ||
| }, [transaction.sender, accounts]) |
There was a problem hiding this comment.
useSigningPipeline already computes signableAddresses via canSignWithAccount() and feeds them into aggregateTransactionWarnings which does the same sender control filtering.
This rederives it independently. I think we can cosume the pipeline's prefiltered warnings instead
| senderAddress: tx.sender, | ||
| targetAddress: closeAddress, | ||
| }) | ||
| const isSenderSignable = signableAddresses.has(tx.sender) |
There was a problem hiding this comment.
signableAddresses uses canSignWithAccount(), which excludes watch accounts. A watched account being closed won't trigger a warning.
Per @wjbeau 's note, should we use all user accounts for warnings and reserve canSignWithAccount() for signing only?
…native into fmsouza/pera-3786
| const closeWarning = warnings.find(w => w.type === 'close') | ||
| const rekeyWarning = warnings.find(w => w.type === 'rekey') |
There was a problem hiding this comment.
Should these maybe be booleans, otherwise you're arbitrarily pickingn one of potentially multiple rekey warnings I guess?
There was a problem hiding this comment.
We're using them to actually show the address in the warning once it shows up on the screen. So if we turn that into a boolean we'll lose this information.
There was a problem hiding this comment.
Ah okay that makes sense so we only use these if there is only one account? Or should that "find" be a "filter" and we deal with a list on the display side?
There was a problem hiding this comment.
Yes, I agree it does make more sense. I've just pushed a change to make it more flexible, so if there are more warnings we show all of them. And then if we don't like how the UX looks like we can definitely review that later.
| senderAddress: tx.sender, | ||
| targetAddress: closeAddress, | ||
| }) | ||
| const isUserAccount = userAccountAddresses.has(tx.sender) |
There was a problem hiding this comment.
I think we need to exclude watch addresses here. Are they already excluded somewhere along the way?
| const closeWarnings = warnings.filter(w => w.type === 'close') | ||
| const rekeyWarnings = warnings.filter(w => w.type === 'rekey') |
There was a problem hiding this comment.
Might be cleaner and more extensible to just group the warnings by type, and then as we add more types the UI can decide which types it cares about? So basically here we create
const warningsByType: Record<'rekey' | 'close' | 'asset-freeze', TransactionWarnings[]> = {
...
}
and then that's what we return?
3 participants
Pull Request Template
Description
Related Issues
Checklist
Additional Notes