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

Implement success/error step 3 of send LSK/BTC - closes #1930 #2005

Merged

Conversation

@osvaldovega
Copy link
Contributor

commented May 13, 2019

What issue have I solved?

-- #1930

How have I implemented/fixed it?

For this implementation I took as based the mobile code as in there this functionality is already implemented.

The send function changed to properly receive the parameters from LSK and BTC and then based on that check the active token and send the parameters to the correct API function.

Now the send function do 2 things create the transactions (Lisk elements -> transfer) and then broadcast the transaction.

How has this been tested?

For test the LSK send functionality I used the send LSK form and confirm that the transaction successfully submit and show up in the wallet transactions table.

For the BTC is necessary mock the BTC data and submit the transaction. Once that the part 1 and 2 of the send request is done supporting BTC, the whole flow needs to be tested.

Review checklist

@osvaldovega osvaldovega self-assigned this May 13, 2019

osvaldovega added some commits May 14, 2019

Merge branch '1930-implement-success-error-step-3-of-send-btc' of git…
…hub.com:LiskHQ/lisk-hub into 1930-implement-success-error-step-3-of-send-btc

osvaldovega added some commits May 16, 2019

@osvaldovega osvaldovega requested a review from massao May 16, 2019

osvaldovega added some commits May 16, 2019

osvaldovega added some commits May 17, 2019

@massao

This comment has been minimized.

Copy link
Contributor

commented May 20, 2019

Check with @Efefefef if he maybe knows the cause of the e2e problem.

osvaldovega added some commits May 20, 2019

Show resolved Hide resolved src/actions/transactions.js
Show resolved Hide resolved src/actions/transactions.js Outdated
Show resolved Hide resolved src/actions/transactions.test.js
Show resolved Hide resolved src/components/sendV2/transactionStatus/transactionStatus.js Outdated
Show resolved Hide resolved src/components/sendV2/transactionStatus/transactionStatus.js Outdated
Show resolved Hide resolved src/utils/api/hwWallet.js Outdated
Show resolved Hide resolved src/utils/api/hwWallet.js Outdated
Show resolved Hide resolved src/utils/api/lsk/transactions.js
Show resolved Hide resolved src/utils/api/lsk/transactions.js Outdated
Show resolved Hide resolved src/components/sendV2/summary/index.js
@osvaldovega

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2019

changes made after feedback. Code updated.

@yasharAyari
Copy link
Member

left a comment

everything is good. there is just a small change needed.

Show resolved Hide resolved src/actions/transactions.js
@massao

This comment has been minimized.

Copy link
Contributor

commented May 21, 2019

@osvaldovega There are still some comments to be fixed, marked as resolved the ones that were fixed.

@massao massao requested a review from yasharAyari May 21, 2019

@yasharAyari yasharAyari requested a review from Efefefef May 22, 2019

@osvaldovega osvaldovega dismissed stale reviews from yasharAyari and massao via c18f3bf May 22, 2019

@Efefefef
Copy link
Contributor

left a comment

🐛 The displayed transaction fee value is not calculated the right way

@osvaldovega osvaldovega requested a review from Efefefef May 22, 2019

@Efefefef
Copy link
Contributor

left a comment

🐛 Open Send form. Fill amount. Check the fee. Click High speed and click back Low speed. The low fee and high fee after switching are different and are not calculated correctly
🐛 Does it make sense to show Transaction fee: 0 BTC if the amount is invalid? I think it should always show the fee value fetched from 3rd party regardless of the amount
🐛 Send page generates an infinite loop of requests
image

@osvaldovega

This comment has been minimized.

Copy link
Contributor Author

commented May 23, 2019

Thx @Efefefef for the review, @massao and I checked and we could found the problem, right now it is fixed and working properly

@osvaldovega osvaldovega requested a review from Efefefef May 23, 2019

@Efefefef
Copy link
Contributor

left a comment

So I request changes, due to ux issue with jumping error messages thaw we discussed with Ali

@osvaldovega

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2019

Yes, this was already check with Ali and this going be handle in a different ticket.
#2068

Massao is working on this UX behavior.

@Efefefef Efefefef added the ready label May 24, 2019

@osvaldovega osvaldovega merged commit dacc38a into development May 24, 2019

3 checks passed

Jenkins e2e tests e2e tests passed
Details
Jenkins test deployment Commit was deployed to test
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details

@osvaldovega osvaldovega deleted the 1930-implement-success-error-step-3-of-send-btc branch May 24, 2019

@reyraa reyraa added this to Pull Requests in Version 1.18.0 via automation May 27, 2019

@reyraa reyraa moved this from Pull Requests to Merged Pull Requests in Version 1.18.0 May 27, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.