-
-
Notifications
You must be signed in to change notification settings - Fork 264
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
Conversation
d30eba5
to
0af25b5
Compare
0af25b5
to
b84f20b
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.
I am not deep in the send here, but it lgtm :) I have a few questions though
suite-common/wallet-core/src/transactions/transactionsThunks.ts
Outdated
Show resolved
Hide resolved
packages/suite/src/hooks/wallet/useCoinmarketRecomposeAndSign.ts
Outdated
Show resolved
Hide resolved
See screenshots, same transaction, Info:
|
QA OK yes fixed and overall OK , i think we can merge it to develop Info:
|
f635113
to
a679c4e
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.
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.
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.
After call I agree that as a prerequisite for send functionality in native app this may make sense. 👍
a679c4e
to
8631fda
Compare
Batch of send form redux logic refactoring. I recommend to review this PR commit by commit.
Description
refactor(suite): signed transaction data stored in redux send form state: signed transaction unserialized data stored in redux state so it does not have to be passed between thunks as an argument
refactor(suite): send form common thunks are rejected on fail: suite-common sendFormThunks utilize redux-toolkit
rejectWithValue
for better error handlingrefactor(suite): send form precomposed transaction arguments naming unified: naming of
precomposedForm
arguments unified for better readabilityrefactor(suite): unify send form precomposed-transaction types naming: There are two types of a successful precomposedTransaction. One for Cardano and other for rest of the networks. This commit makes the naming of these types clearer.
refactor(suite): split pushSendFormTransactionThunk into sub-thunks: Bulky pushSendFormTransactionThunk was split into smaller sub-thunks. Some of theme were needed only by desktop, these were moved to
packages/suite
package.refactor(suite): convert send form drafts amount units refactored
refactor(suite): redundant precomposedForm state removed from sendFormReducer