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.

Sign up for GitHub

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

Jump to bottom

refactor: SignOrExecute elements #1842

Merged
merged 11 commits into from
Apr 24, 2023
Merged

refactor: SignOrExecute elements #1842

merged 11 commits into from
Apr 24, 2023

Conversation

katspaugh
Copy link
Member

@katspaugh katspaugh commented Apr 4, 2023 •
edited
Loading

What it solves

Part of #1828

An umbrella PR for code refactors in preparation for a tx flow redesign.

How to test

Make all kinds of transactions, immediate executions/signatures/on-chain signatures/relayed executions. Everything should work like before.

Analytics changes

Relayed executions now have their own event instead of "Propose".

@katspaugh
Refactor: extract SignOrExecute logic into hooks (#1830)
1595cc7 
* Refactor: extract SignOrExecute logic into hooks

* isExecutionLoop

* Fix isExecutionLoop

* Add hook tests

* PR comments

* Create asserts

* Use isValidMasterCopy

* Track relayed executions

* Tests
@katspaugh
Refactor: move getRemainingRelays to a service (#1839)
810a22a 
* Refactor: move getRemainingRelays to a service

* Rename relaying service and constants
@github-actions
Copy link

github-actions bot commented Apr 4, 2023 •
edited
Loading

Branch preview

✅ Deploy successful!

https://tx_refactor--webcore.review-web-core.5afe.dev

@github-actions
Copy link

github-actions bot commented Apr 4, 2023 •
edited
Loading

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 mentioned this pull request Apr 5, 2023
Merged
3 tasks
@katspaugh katspaugh mentioned this pull request Apr 5, 2023
Merged
3 tasks
@iamacook
Merge branch 'dev' into tx-refactor
b361689
@iamacook
refactor: address relaying review comments (#1848)
e5db84e 
* refactor: address relaying review comments

* fix: revert type error expectation

* fix: add help article link

* fix: spelling mistake
@iamacook iamacook changed the title [Epic] Tx flow refactor refactor: SignOrExecute elements Apr 12, 2023
@liliya-soroka
Copy link
Member

liliya-soroka commented Apr 19, 2023 •
edited
Loading

  • I am getting this error from time to time on sign+execute or sign without execution. When the submit is clicked the second time, the signing or execution passes without any problem

image

I am facing the same issue on app.safe.global on Gnosis chain only. - Not relayed to the current PR

@liliya-soroka
Copy link
Member

liliya-soroka commented Apr 19, 2023 •
edited
Loading

Screen.Recording.2023-04-19.at.14.36.18.mov

Steps for the tx creation:

  1. open tx build
  2. add safe address to the "enter address" field
  3. click "keep proxy ABI"
  4. enter amount = 0 => add tx
  5. add a few such txs and create a batch tx using tx builder

@usame-algan
Copy link
Member

The app crashes when I try to execute - https://tx_refactor--webcore.review-web-core.5afe.dev/transactions/tx?safe=gno:0x9DF58F04F3e914ED8D972146401C85313A53FecA&id=multisig_0x9DF58F04F3e914ED8D972146401C85313A53FecA_0xf46cbeeb673201593a8ffae2caa7ffb7877fc3671599c96f6135993271e924b9

Looks like data can be null which is not reflected in our types.

@liliya-soroka can you provide the steps in how you created that transaction?

@liliya-soroka
Copy link
Member

@usame-algan , added to the comment with the issue

@usame-algan
Copy link
Member

@liliya-soroka I was able to reproduce the bug on dev as well. I suggest to fix it there first. Will create an issue for it.

@usame-algan usame-algan mentioned this pull request Apr 20, 2023
Closed
@liliya-soroka
Copy link
Member

liliya-soroka commented Apr 20, 2023 •
edited
Loading

What was tested:

  1. Execution using relay on Gnosis chain
  2. Execution when relay tx is 0 of 5 -> execution with the owner
  3. outgoing transfer /transferfrom
  4. spending limits /module txs
  5. settings txs
  6. safe apps txs with diff amounts and types of actions - Unhandled error when decoded data is null #1890
  7. sign + execute in N of M
  8. execute in N of M
  9. sign + execute in 1 of M
  10. execute in 1 of M
  11. batch tx execution without relay
  12. batch tx execution with relay - we have a separate issue - 500 error during batch tx execution using relay  #1891

@usame-algan usame-algan mentioned this pull request Apr 20, 2023
Merged
3 tasks
@katspaugh katspaugh merged commit 3a0b6af into dev Apr 24, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Apr 24, 2023
@katspaugh katspaugh deleted the tx-refactor branch November 23, 2023 13:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@usame-algan usame-algan usame-algan approved these changes

@iamacook iamacook iamacook approved these changes

Assignees
No one assigned
Labels
None yet
Projects
Web Squad
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@katspaugh @liliya-soroka @usame-algan @iamacook