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

Feat/native/review and send #12968

Merged
merged 2 commits into from
Jul 2, 2024
Merged

Feat/native/review and send #12968

merged 2 commits into from
Jul 2, 2024

Conversation

PeKne
Copy link
Contributor

@PeKne PeKne commented Jun 19, 2024 •
edited
Loading

Enables basic send form submission of BTC testnets (TEST and REGTEST). While reviewing this, please focus on the design of the whole flow and code architecture. Reviewing UI or some styling/translation nits is not necessary at this stage, because this implementation does not reflect the final design. The purpose of this PR is solely to prepare the codebase so it can send the transaction.

Also there are some steps of the send flow that are not implemented yet but will be revisited in follow up PRs (eg. fee selection, multiple recipients...)

Description

The PR is divided into two commits:

  • 5d693e5- moves transaction outputs utils from packages to suite-common to be reused in native. There are no new things implemented only moving stuff around.
  • f6df275 - transaction review & send
    • Review screen reflecting on-device confirmation steps added
    • basic error handling implemented
    • on successful on-device verification, the user can click the send transaction button and broadcast the transaction to the blockchain
    • note: sorry for making this PR so big, but most of it is just UI so I hope it's not that big a deal. In case you need a better explanation of this PR, ping me on Slack

Related Issues

Resolve #10867
Resolve #10868

Screenshots:

Screen.Recording.2024-06-19.at.13.27.00.mov

@PeKne PeKne force-pushed the feat/native/review-and-send branch from 53bb629 to f6df275 Compare June 19, 2024 12:31
@PeKne PeKne marked this pull request as ready for review June 19, 2024 12:40
@PeKne PeKne requested a review from a team as a code owner June 19, 2024 12:40
@PeKne PeKne changed the title WIP: Feat/native/review and send Feat/native/review and send Jun 19, 2024
@juriczech juriczech self-assigned this Jun 24, 2024
juriczech
juriczech suggested changes Jun 24, 2024
View reviewed changes
Copy link
Contributor

@juriczech juriczech left a comment

Choose a reason for hiding this comment

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

Overall good job! Just some things I'd rather get right from the beginning rather than creating code that will be refactored. I'm sorry if this sounds the wrong way, but IMO in the future, we shouldn't create PR this size with saying most of it will change. If I'm reviewing some code, I'd like to get it right to build on top of it or don't do it in a PR like this. Becuase then it might never be changed and if we don't review it then it's a recipe for unmaintainability. However it's just some things for discussion so let's see if they're important or not. Feel free to huddle me if you want to further discuss them.

.../components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewModalContent.tsx Outdated Show resolved Hide resolved
suite-common/wallet-types/src/transaction.ts Show resolved Hide resolved
suite-common/wallet-utils/src/reviewTransactionUtils.ts Outdated Show resolved Hide resolved
packages/suite/src/types/wallet/transaction.ts Outdated Show resolved Hide resolved
suite-common/wallet-core/src/device/deviceReducer.ts Outdated Show resolved Hide resolved
suite-native/module-send/src/components/SendOutputsForm.tsx Show resolved Hide resolved
suite-native/module-send/src/screens/SendOutputsScreen.tsx Show resolved Hide resolved
suite-native/module-send/src/screens/SendReviewScreen.tsx Outdated Show resolved Hide resolved
suite-native/module-send/src/screens/SendReviewScreen.tsx Outdated Show resolved Hide resolved
suite-native/navigation/src/components/GoBackIcon.tsx Outdated Show resolved Hide resolved
@juriczech
Copy link
Contributor

This pull request can be closed as redundant now? #12240

@juriczech juriczech added the mobile Suite Lite issues and PRs label Jun 24, 2024
@PeKne PeKne mentioned this pull request Jun 24, 2024
Closed
Nodonisko
Nodonisko reviewed Jun 25, 2024
View reviewed changes
Copy link
Contributor

@Nodonisko Nodonisko left a comment

Choose a reason for hiding this comment

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

Seems that @juriczech already pointed out most of issues, only one thing that I have in mind is that we should call updateFeeInfoThunk in the begging of send flow because on mobile we don't update fee with every block like on desktop.

@PeKne PeKne force-pushed the feat/native/review-and-send branch from 3b8cbb2 to affdc58 Compare July 2, 2024 07:39
@PeKne PeKne force-pushed the feat/native/review-and-send branch from cba8fb0 to 77256e6 Compare July 2, 2024 11:32
@PeKne
Copy link
Contributor Author

PeKne commented Jul 2, 2024

@Nodonisko the updateFeeInfoThunk call added ✅

@PeKne PeKne merged commit 1349e42 into develop Jul 2, 2024
85 of 86 checks passed
@PeKne PeKne deleted the feat/native/review-and-send branch July 2, 2024 13:06
@komret komret mentioned this pull request Jul 25, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Nodonisko Nodonisko Nodonisko left review comments

@juriczech juriczech juriczech approved these changes

Assignees

@juriczech juriczech

Labels
mobile Suite Lite issues and PRs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Send coins - send transaction Send coins - inputs review screen
3 participants
@PeKne @juriczech @Nodonisko