-
Notifications
You must be signed in to change notification settings - Fork 360
refactor: use TxModalWrapper in ApproveTxModal #3369
Conversation
CLA Assistant Lite All Contributors have signed the CLA. |
ESLint Summary View Full Report
Report generated by eslint-plus-action |
8c34603
to
8089432
Compare
E2E Tests Failed Failed tests:
|
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.
It's great to see how much code has been removed but I'm cautious of how much state you've removed. I think @DiogoSoaress will give a better insight than me.
src/routes/safe/components/Transactions/helpers/EditTxParametersForm/index.tsx
Outdated
Show resolved
Hide resolved
src/routes/safe/components/Transactions/helpers/EditableTxParameters.tsx
Show resolved
Hide resolved
src/routes/safe/components/Transactions/helpers/EditableTxParameters.tsx
Outdated
Show resolved
Hide resolved
src/routes/safe/components/Transactions/helpers/TxModalWrapper/index.tsx
Outdated
Show resolved
Hide resolved
src/routes/safe/components/Transactions/helpers/TxParametersDetail/index.tsx
Show resolved
Hide resolved
e18204a
to
d79b192
Compare
src/routes/safe/components/Transactions/helpers/TxModalWrapper/index.tsx
Outdated
Show resolved
Hide resolved
src/routes/safe/components/Transactions/helpers/TxParametersDetail/index.tsx
Show resolved
Hide resolved
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.
This looks good to me now but I would wait to see if @DiogoSoaress agrees
src/routes/safe/components/Transactions/helpers/TxModalWrapper/index.tsx
Outdated
Show resolved
Hide resolved
src/routes/safe/components/Transactions/TxList/modals/ApproveTxModal.tsx
Show resolved
Hide resolved
PR comments Show only nonce when creating a multisig tx Revert parametersStatus changes Partially revert #3257 (it was buggy) Rm snackbar Onchain sig or execution
d47dcda
to
b30f400
Compare
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.
👍
One Modal (wrapper) to rule them all.
e6d0c59
to
12ac12e
Compare
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.
LGTM!
This comment has been minimized.
This comment has been minimized.
In a safe 3/x I try to do the last signature to execute the tx, but I get the error that the of "invalid owner provided" even tho I'm connected with an owner of the safe The safe: https://pr3369--safereact.review-safe.gnosisdev.com/app/rin:0x37A53791b3667c803318D083D339C236781D57D1/transactions/queue |
@francovenica you sure those things wouldn't happen on dev? |
This one also only happens in this PR, in dev I don't get the "Invalid owner provided" when I try to confirm with the last owner. |
The issue in the tx builder, of queuing several tx and getting the warning message yes, that one also happens in dev. I'll report it separately |
@francovenica Diogo and I have fixed the two bugs you found. The root cause was we forgot to pass safeTxGas, so it was 0. |
Both issues were fixed
Looks good to me |
Builds on top of #3297.
ApproveTxModal
is very similar to all other modals but needed a few extra props to be passed to TxModalWrapper.useCanTxExecute
I've reverted parts of #3257.
Namely, the logic that checks the nonce. It caused bugs and was triggered for just 1/1 Safes anyway, so practically useless.
The new logic is that it only checks that either a) there are enough sigs b) there's one sig missing but an owner is executing the tx.
Behavior before this change:
If an owner of a 1/1 safe creates a tx with the nonce out of order, execution checkbox won't be shown.
After this change:
Checkbox is always shown but tx will be put to the queue if nonce is not the current safe nonce.
Nothing changes for n/n+m Safes.
Links
Refactoring rationale and a UI flow diagram: https://kinopio.club/refactoring-tx-modals-SoJbXclHVCFNCK2R36Jsi