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

Enable store and send BTC feature - Closes #1893 #2150

Merged
merged 65 commits into from Jun 28, 2019

Conversation

Projects
None yet
4 participants
@slaweet
Copy link
Member

commented Jun 18, 2019

What issue have I solved?

#1893

How have I implemented/fixed it?

  • Removed all the localStorage.getItem('btc') conditions
  • Updated unit tests to pass
  • Fix some styles on the transaction page, especially in smaller breapoints
  • Fix onClick of CopyToClibpboard
  • Fix tabs styles for when first tabs is also last
  • Transaction list in wallet gets sometimes LSK transactions even if BTC is active
  • Prevent an error on wallet after sending BTC
  • Adjust "Empty account" wallet module to adjust to active token.
  • Update src/utils/api/btc/transactions.js to work also with BTC mainnet

How has this been tested?

Try to

  • Send BTC
  • Switch token
  • Enable/disable BTC
  • View BTC transactions in wallet and dashboard

Review checklist

@slaweet slaweet self-assigned this Jun 19, 2019

@slaweet slaweet force-pushed the 1893-enable-btc-to-end-users branch 2 times, most recently from b923549 to 60e4034 Jun 19, 2019

@slaweet slaweet changed the base branch from 2053-implement-new-design-of-main-header to development Jun 19, 2019

@slaweet slaweet force-pushed the 1893-enable-btc-to-end-users branch 3 times, most recently from d0e26bd to e7a6832 Jun 20, 2019

@slaweet slaweet marked this pull request as ready for review Jun 21, 2019

@slaweet slaweet requested a review from massao Jun 21, 2019

@massao
Copy link
Contributor

left a comment

🐛 After sending LSK or BTC, if you add to bookmark the dropdown state isn't being updated properly so it's possible to add multiple times the same address.

Show resolved Hide resolved src/actions/account.js Outdated
Show resolved Hide resolved src/actions/account.js Outdated
@slaweet

This comment has been minimized.

Copy link
Member Author

commented Jun 21, 2019

Thank you for the feedback @massao, good ideas. Fixes pushed, please check again.

@slaweet slaweet requested a review from massao Jun 21, 2019

@massao
Copy link
Contributor

left a comment

👍

@massao massao requested a review from Efefefef Jun 21, 2019

slaweet added some commits Jun 18, 2019

🐛 Fix btc transactions utils to use given network
to make it work for both mainnet and testnet
@slaweet

This comment has been minimized.

Copy link
Member Author

commented Jun 26, 2019

@Efefefef ready to check:

  • Disable account initialization for BTC
  • Disable BTC on HW wallet Login
  • Redirect from non-BTC pages to Dashboard on token switch

@yasharAyari what should happen if btc-test.lisk.io is unavailable on login? Login and disable BTC?

@Efefefef
Copy link
Contributor

left a comment

🐛 Transfer tx with HW wallet doesn't work

@slaweet slaweet requested a review from yasharAyari Jun 27, 2019

@slaweet

This comment has been minimized.

Copy link
Member Author

commented Jun 27, 2019

Jenkins green now. We tested my send with hw wallet fix with @yasharAyari
Anything else @Efefefef ?

@Efefefef

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2019

@slaweet Yes, I'm waiting for the answers from @yasharAyari for questions above
#2150 (review)
Here 2 mentions. Please comment

@Efefefef Efefefef self-requested a review Jun 27, 2019

@Efefefef
Copy link
Contributor

left a comment

@slaweet For me it's still unclear. Why when I make BTC transfer then go to wallet and cant see transaction in there for a whole minute. Cant we show it regardless of data fetched?

@slaweet

This comment has been minimized.

Copy link
Member Author

commented Jun 27, 2019

@slaweet For me it's still unclear. Why when I make BTC transfer then go to wallet and cant see transaction in there for a whole minute. Cant we show it regardless of data fetched?

@Efefefef, We can show it. I also think it's better to show it. It should be as easy as removing this if

if (activeToken !== tokenMap.BTC.key) {
but @yasharAyari had explicitly asked me to create a ticket #2127 to put that condition in place.

Both @yasharAyari and @reyraa yesterday confirmed that not handling pending transactions locally is expected behavior for BTC common in other BTC wallets.

@yasharAyari

This comment has been minimized.

Copy link
Member

commented Jun 27, 2019

@Efefefef about the first question, we can't keep the PR open because of a bug in the BTC-bridge. As soon as they fix the issue, we will patch the lisk-hub to work with the changes.
for the second problem please open an issue because it is not related to the context of this PR.

@Efefefef

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2019

@yasharAyari Question is if we can release BTC feature with this bug, not about keeping PR open. How we are going to track the fix?
For the second one I created a ticket #2197

@Efefefef

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2019

Ok then, the last question would be about Search functionality in BTC mode.
I could not find a single word about it in the epic. Should we disable it?

@slaweet

This comment has been minimized.

Copy link
Member Author

commented Jun 27, 2019

Checked with @yasharAyari:

  • Search icon in top bar should be hidden for BTC (commit coming soon)

  • the bug in bitcoin bridge happens for lisk-mobile, so it’s ok to release

And I add that, the user needs to have over 30 transactions for the bug to happen, so it's not that likely to happen.

@yasharAyari

This comment has been minimized.

Copy link
Member

commented Jun 27, 2019

@Efefefef BTC-bridge is stabled enough to be used in production.it has used in the lisk-mobile already.

slaweet added some commits Jun 27, 2019

@Efefefef

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2019

@slaweet Please make the LSK accounts, transactions, and the rest unavailable and redirected under active BTC

@slaweet slaweet force-pushed the 1893-enable-btc-to-end-users branch from b6877f4 to e9af909 Jun 28, 2019

@slaweet slaweet requested a review from Efefefef Jun 28, 2019

@Efefefef
Copy link
Contributor

left a comment

🏅

@Efefefef Efefefef added the ready label Jun 28, 2019

@slaweet slaweet merged commit 869f26c into development Jun 28, 2019

4 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
coverage/coveralls Coverage increased (+0.006%) to 94.67%
Details

@slaweet slaweet deleted the 1893-enable-btc-to-end-users branch Jun 28, 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.