-
Notifications
You must be signed in to change notification settings - Fork 454
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: Add batch execute button and tx highlighting #352
Conversation
ESLint Summary View Full Report
Report generated by eslint-plus-action |
Deploying with Cloudflare Pages
|
import { styled } from '@mui/material/styles' | ||
import Tooltip, { tooltipClasses, TooltipProps } from '@mui/material/Tooltip' | ||
|
||
const CustomTooltip = styled(({ className, ...props }: TooltipProps) => ( |
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.
Only extracted this so it can be reused
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.
Amazing work dude 💪
src/components/transactions/BatchExecuteButton/__tests__/BatchExecute.test.tsx
Outdated
Show resolved
Hide resolved
src/components/transactions/TxListItem/ExpandableTransactionItem.tsx
Outdated
Show resolved
Hide resolved
batchableTransactions.length < BATCH_LIMIT && | ||
isMultisigExecutionInfo(tx.transaction.executionInfo) && | ||
tx.transaction.executionInfo.nonce === currentNonce && | ||
tx.transaction.executionInfo.confirmationsSubmitted >= tx.transaction.executionInfo.confirmationsRequired |
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.
I know its just a minor bug but lets add a condition here that we stop adding txs to the batch if an update safe tx was added.
5afe/safe-react#4014
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.
As discussed lets tackle this in a separate task as it involves decoding each tx which could blow up the scope
import { selectPendingTxs } from '@/store/pendingTxsSlice' | ||
import CustomTooltip from '@/components/common/CustomTooltip' | ||
|
||
const BATCH_LIMIT = 10 |
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.
I know it was 10 txs before. But I'm not 100% sure where the 10 is coming from. Is it about the block gas limit?
If yes: Some apps like tx builder / csv airdrop can by itself create huge txs. 10 of those would not fit.
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.
As far as I remember we chose 10 arbitrarily. By default we get 20 per page now I think so we could increase it to that
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.
Actually now infinite scroll makes more sense. 🤔
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.
Let's go! Nice work! 🚀
What it solves
ToDos
Screenshots