Skip to content
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: actionable pending txs on the Dashboard #2523

Merged
merged 6 commits into from
Sep 21, 2023
Merged

Feat: actionable pending txs on the Dashboard #2523

merged 6 commits into from
Sep 21, 2023

Conversation

katspaugh
Copy link
Member

What it solves

Part of #2510

How this PR fixes it

Shows only signable and executable queued txs, not just any queued txs. Also displays a ✔️ or 🚀 button if a tx can be executed or signed.

Screenshot 2023-09-18 at 17 25 00

@github-actions
Copy link

github-actions bot commented Sep 18, 2023

Branch preview

✅ Deploy successful!

https://pending--walletweb.review-wallet-web.5afe.dev

@github-actions
Copy link

github-actions bot commented Sep 18, 2023

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

@katspaugh katspaugh marked this pull request as draft September 18, 2023 15:43
@katspaugh katspaugh marked this pull request as ready for review September 19, 2023 07:20
Comment on lines 47 to 55
const actionable = useMemo(() => {
return wallet
? queuedTxns.filter(
(tx) => isSignableBy(tx.transaction, wallet.address) || isExecutable(tx.transaction, wallet.address, safe),
)
: queuedTxns
}, [wallet, queuedTxns, safe])

const txs = actionable.length ? actionable : queuedTxns
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

Suggested change
const actionable = useMemo(() => {
return wallet
? queuedTxns.filter(
(tx) => isSignableBy(tx.transaction, wallet.address) || isExecutable(tx.transaction, wallet.address, safe),
)
: queuedTxns
}, [wallet, queuedTxns, safe])
const txs = actionable.length ? actionable : queuedTxns
const actionableTxns = useMemo(() => {
return wallet
? queuedTxns.filter(
(tx) => isSignableBy(tx.transaction, wallet.address) || isExecutable(tx.transaction, wallet.address, safe),
)
: queuedTxns
}, [wallet, queuedTxns, safe])
const txs = actionable.length ? actionableTxns : queuedTxns

Copy link
Member

@usame-algan usame-algan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one 💯

@francovenica
Copy link
Contributor

Looks goood to me:

I tested in a safe 3/3:
The users can only see transactions that they can confirm.
They can see tx that are 1 signature away from execution and the icon shown is the "rocket" to execute right away.
A fully signed tx shows up again with the "Rocket" icon to execute

@francovenica
Copy link
Contributor

Reopening this one because of an issue found:

I created a tx with an owner and he can see the tx in the list. Is expected that he shouldn't see the tx since he already signed during creation

The safe:
https://pending--walletweb.review-wallet-web.5afe.dev/home?safe=gor:0xA601D7b958A7Cdd82B5Cf5230D6C40876E9e27C4

The owner: (see if impersonator works)
gor:0x7724b234c9099C205F03b458944942bcEBA13408

Gif:
tx list

@katspaugh
Copy link
Member Author

When there are no signable/executable txs, it will display any queued txs, so it's expected.

@katspaugh katspaugh merged commit 7637b32 into dev Sep 21, 2023
9 checks passed
@katspaugh katspaugh deleted the pending branch September 21, 2023 20:25
@github-actions github-actions bot locked and limited conversation to collaborators Sep 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants