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

Refactor/send form redux cleanup #12378

Merged
merged 7 commits into from
May 29, 2024
Merged

Refactor/send form redux cleanup #12378

merged 7 commits into from
May 29, 2024

Conversation

PeKne
Copy link
Contributor

@PeKne PeKne commented May 14, 2024

Batch of send form redux logic refactoring. I recommend to review this PR commit by commit.

Description

@PeKne PeKne force-pushed the refactor/send-form-cleanup branch from d30eba5 to 0af25b5 Compare May 14, 2024 13:16
@PeKne PeKne changed the title Refactor/send form cleanup Refactor/send form redux cleanup May 14, 2024
@PeKne PeKne force-pushed the refactor/send-form-cleanup branch from 0af25b5 to b84f20b Compare May 14, 2024 13:31
@PeKne PeKne requested a review from a team May 14, 2024 13:59
@PeKne PeKne marked this pull request as ready for review May 14, 2024 15:42
@PeKne PeKne requested a review from marekrjpolak as a code owner May 14, 2024 15:42
Copy link
Contributor

@vytick vytick left a comment

Choose a reason for hiding this comment

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

I am not deep in the send here, but it lgtm :) I have a few questions though

@PeKne PeKne added the build-desktop This will trigger the build of desktop apps for your PR label May 15, 2024
packages/suite/src/actions/wallet/send/sendFormThunks.ts Outdated Show resolved Hide resolved
packages/suite/src/actions/wallet/send/sendFormThunks.ts Outdated Show resolved Hide resolved
packages/suite/src/actions/wallet/send/sendFormThunks.ts Outdated Show resolved Hide resolved
packages/suite/src/actions/wallet/send/sendFormThunks.ts Outdated Show resolved Hide resolved
packages/suite/src/actions/wallet/send/sendFormThunks.ts Outdated Show resolved Hide resolved
packages/suite/src/hooks/wallet/form/useCompose.ts Outdated Show resolved Hide resolved
packages/suite/src/hooks/wallet/useSendForm.ts Outdated Show resolved Hide resolved
@bosomt
Copy link
Contributor

bosomt commented May 17, 2024

See screenshots, same transaction, RBF on but after tx is signed modal reports RBF off

Screenshot 2024-05-17 at 11 28 58 Screenshot 2024-05-17 at 11 29 07

Info:

  • Suite version: desktop 24.6.0 (a6fe7cd)
  • Browser: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) TrezorSuiteDev/24.6.0 Chrome/118.0.5993.159 Electron/27.3.8 Safari/537.36
  • OS: MacIntel
  • Screen: 1512x982
  • Device: Trezor T2T1 2.7.0 regular (revision 45e8a842a31e62a6d43d7f6ccac62a45e1198ef0)
  • Transport: BridgeTransport 3.0.0

@PeKne
Copy link
Contributor Author

PeKne commented May 20, 2024

@bosomt The problem should be fixed by: f635113

Please test it again 🙏.

@bosomt
Copy link
Contributor

bosomt commented May 20, 2024

QA OK

yes fixed and overall OK , i think we can merge it to develop

Info:

  • Suite version: web 24.6.0 (31b59dc)
  • Browser: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:126.0) Gecko/20100101 Firefox/126.0
  • OS: MacIntel
  • Screen: 1512x982
  • Device: Trezor T2T1 2.7.0 regular (revision 45e8a842a31e62a6d43d7f6ccac62a45e1198ef0)
  • Transport: BridgeTransport 2.0.33

@PeKne PeKne force-pushed the refactor/send-form-cleanup branch from f635113 to a679c4e Compare May 22, 2024 07:05
Copy link
Contributor

@marekrjpolak marekrjpolak left a comment

Choose a reason for hiding this comment

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

Looks good in general, a lot of work has been done here.

The only thing I'm not quite happy about is the first commit: I prefer passing required arguments explicitly between the stages of a single process, rather than using the global state for it. That way:

  • data validation is performed by type checking in development time instead of runtime checks
  • functions (mostly thunks in this case) have as few side effects as possible while having as few dependencies as possible at the same time
  • less code is needed to pass an argument than to store it into the state, read it somewhere else and validate that it is in fact there
  • less effort is needed to understand the code if it's directly visible what the function needs in order to work and what it produces without looking into its implementation (that's why pure functions are so beautiful)

In this case it seems that signedTx and serializedTx are both stored at the beginning of signAndPushSendFormTransactionThunk (or its subroutine) and needed somewhere at the end of it, therefore I don't see a reason for using global state other than hiding the complexity of passing the parameters, which makes the overall complexity bigger instead of smaller imo.

I'm not saying it cannot be merged as it is, but I'll try to look at it a bit more if you don't mind.

Copy link
Contributor

@marekrjpolak marekrjpolak left a comment

Choose a reason for hiding this comment

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

After call I agree that as a prerequisite for send functionality in native app this may make sense. 👍 :shipit:

@PeKne PeKne force-pushed the refactor/send-form-cleanup branch from a679c4e to 8631fda Compare May 29, 2024 14:10
@PeKne PeKne merged commit 337fabb into develop May 29, 2024
27 checks passed
@PeKne PeKne deleted the refactor/send-form-cleanup branch May 29, 2024 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build-desktop This will trigger the build of desktop apps for your PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants