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: batch any tx #2254

Merged
merged 29 commits into from
Aug 2, 2023
Merged

Feat: batch any tx #2254

merged 29 commits into from
Aug 2, 2023

Conversation

katspaugh
Copy link
Member

@katspaugh katspaugh commented Jul 7, 2023

Project links

Design: https://www.figma.com/file/FY2LtTxP2Ngj6Ed5dTTDcv/Batching?type=design&node-id=6%3A30859&mode=design&t=svhcdpjFCmNn9yMN-1
Specs: https://www.notion.so/Transaction-batching-9600b8fa53834512ba0c8d1a257de771

Description

  • You can now choose to batch any created tx instead of signing
  • A new sidebar shows batched txs
  • You can add new txs or sign the batch

Todo

  • Counter on the Batch icon
  • Batch Icon is black in dark mode
  • Expand txs to see details
  • Mobile version
  • Overall design as per figma
  • Batching multiSends does not work, we have to flatten multiSends and add all the “inner transactions”, because we cannot / do not want to delegateCall – batching multisend txs is disabled for now
  • The option to batch is hidden when executing, which is the default for 1/x Safe
Screenshot 2023-07-07 at 21 35 09

@katspaugh katspaugh marked this pull request as draft July 7, 2023 16:12
@github-actions
Copy link

github-actions bot commented Jul 7, 2023

Branch preview

✅ Deploy successful!

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

@github-actions
Copy link

github-actions bot commented Jul 7, 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

@github-actions
Copy link

github-actions bot commented Jul 23, 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 requested a review from schmanu July 24, 2023 12:58
@katspaugh katspaugh marked this pull request as ready for review July 24, 2023 12:58
@katspaugh katspaugh marked this pull request as draft July 25, 2023 13:38
@katspaugh katspaugh marked this pull request as ready for review July 26, 2023 14:38
src/components/batch/BatchSidebar/BatchTxList.tsx Outdated Show resolved Hide resolved
src/hooks/useDraftBatch.ts Outdated Show resolved Hide resolved
src/hooks/useDraftBatch.ts Outdated Show resolved Hide resolved
src/components/batch/BatchSidebar/BatchTxItem.tsx Outdated Show resolved Hide resolved
src/components/batch/BatchSidebar/BatchTxItem.tsx Outdated Show resolved Hide resolved
src/components/batch/BatchSidebar/EmptyBatch.tsx Outdated Show resolved Hide resolved
src/components/batch/BatchSidebar/EmptyBatch.tsx Outdated Show resolved Hide resolved
Copy link
Member

@schmanu schmanu left a comment

Choose a reason for hiding this comment

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

Looks and works really great!

I just have some minor suggestions and there are some prettier problems in BatchTxItem.tsx

src/components/tx/SignOrExecuteForm/index.tsx Outdated Show resolved Hide resolved
src/services/tx/tx-sender/__tests__/ts-sender.test.ts Outdated Show resolved Hide resolved
src/store/batchSlice.ts Outdated Show resolved Hide resolved
src/store/batchSlice.ts Outdated Show resolved Hide resolved
@katspaugh katspaugh marked this pull request as draft July 27, 2023 09:59
Copy link
Member

@iamacook iamacook left a comment

Choose a reason for hiding this comment

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

There are some Prettier issues, but otherwise looks good!

src/components/batch/BatchIndicator/BatchTooltip.tsx Outdated Show resolved Hide resolved
@katspaugh katspaugh force-pushed the batching branch 2 times, most recently from 7f7b2cc to 3663f32 Compare July 28, 2023 06:13
@katspaugh katspaugh marked this pull request as ready for review July 28, 2023 10:37
@francovenica
Copy link
Contributor

I don't think the "Send again" should be in the list of batched txs
image

@francovenica
Copy link
Contributor

francovenica commented Aug 1, 2023

Rejections should not be "batchable".
First you can put the same rejection more than once, which should not be possible in a regular tx.
Second, the only tx you could reject is the batch you are creating itself or a tx that has a nonce -1 of the batch you are creating, therefore you would try to reject a tx that has already being executed or rejected
image

@katspaugh
Copy link
Member Author

I don't think the "Send again" should be in the list of batched txs

It's difficult to hide it just in that sidebar, so I suggest we keep it, it doesn't hurt. I made it close the batch sidebar when Send again is pressed.

Rejections should not be "batchable".

Fixed! ✅

@francovenica
Copy link
Contributor

francovenica commented Aug 1, 2023

The event "Expand batched tx" is being triggered also when you are trying to delete a tx from the batch or reordering it, because the event triggers as soon as you click on a tx and not when the tx details actually display.
EDIT: it triggers when you release the click, but still, it should trigger when the tx details are displayed

image

@francovenica
Copy link
Contributor

Suggestion: The Add to batch button that is at the bottom of the tx form only shows if you click the "no execution" checkbox. The thing is that if the user have not noticed the icon at the top he will not know about batch tx until someday he clicks in the "no execution". So I'd recomend show that button all the time

@katspaugh
Copy link
Member Author

katspaugh commented Aug 1, 2023

I’ll consider it for v2. Manu also pointed out that 1/N users who chose to execute txs by default will not see this button.

@francovenica
Copy link
Contributor

francovenica commented Aug 1, 2023

Another Nish one: If in the tx builder you only set 1 tx and try to execute it you can batch it (which is fine) but after batching the app shows the modal of "Success".
I'd assume that we should see how apps behave with the batching. Or maybe we can close the app as soon as a tx is batched
image

@francovenica
Copy link
Contributor

The button is overlapping in mobile viewports
image

@francovenica
Copy link
Contributor

francovenica commented Aug 1, 2023

Also for mobile, the elements are too wide for it. This is emulating a iPhone 12. I tried in a physical iphone13 and is the same.
The numbers and the arrow to display it cannot be seen

image

@francovenica
Copy link
Contributor

Question. In one of the designs is implied that you can reorder and remove tx from the batch in the form, but in other parts is not. So, should the user be able to do that in the form?
image

I've seen that, as you reorder/remove items from the batch sidebar they are also being removed/reorder in the form, so that would be fine too, but probably should put some info text saying "You can reorder/remove tx from the sideber" if we are not going to allow it in the form.

@francovenica
Copy link
Contributor

Note:
Although Safe apps and Spending limits can be batched, usually those type of tx are already a batch, so the vast majority of those type of tx will not be batchable, we should see if it's worth having the button there for those

@katspaugh
Copy link
Member Author

I'll disable this feature on mobile for v1.

@katspaugh
Copy link
Member Author

Question. In one of the designs is implied that you can reorder and remove tx from the batch in the form, but in other parts is not. So, should the user be able to do that in the form?

For now they'll be re-orderable/deletable only in the Sidebar view. The designs are a mix of v1 and v2.

@katspaugh
Copy link
Member Author

Delegate/batch txs currently have a disabled "Add to batch" button as per @kirkkonen's request. It shows a tooltip on hover.

I'm going to merge this PR, and we'll address the rest of the feedback in v2. Namely:

  • Mobile version
  • Post-submit state in Safe Apps
  • Batch button in the Execute form

@katspaugh katspaugh merged commit 661bafd into dev Aug 2, 2023
6 of 7 checks passed
@katspaugh katspaugh deleted the batching branch August 2, 2023 13:53
@github-actions github-actions bot locked and limited conversation to collaborators Aug 2, 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.

4 participants