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 btc bookmarks - Closes #1962 #1989

Merged
merged 20 commits into from May 14, 2019

Conversation

Projects
None yet
2 participants
@massao
Copy link
Contributor

commented May 8, 2019

What issue have I solved?

#1962

How have I implemented/fixed it?

  • Updated followed accounts to be able to store LSK and BTC addresses;
  • Updated all components using the followed accounts;
  • Created flattenFollowedAccounts to generate a single level array with all the accounts;
  • Updated all unit tests related to followed accounts;
  • Removed components that were using followed accounts but weren't being used on the application;
  • Had to put a validation on walletTab component to hide account details when account is BTC, should be removed/improved on #2003.

How has this been tested?

  1. Enable BTC with localStorage.setItem('btc', true);
  2. Go to any BTC account page. eg /explorer/accounts/mkakDp2f31btaXdATtAogoqwXcdx1PqqFo;
  3. Add to bookmark through the dropdown;
  4. Go to dashboard and check that the account shows up on the followed accounts module;
  5. Try editing and removing the bookmark.

Review checklist

massao added some commits May 6, 2019

@massao massao self-assigned this May 8, 2019

massao added some commits May 10, 2019

@massao massao requested review from michaeltomasik and slaweet May 13, 2019

@slaweet
Copy link
Member

left a comment

There are some things to clarify:

  • The issue says “Specification and designs to be added.” #1962 Would be nice to have the ticket updated to provide that info
  • The issue label is type:code, so it shouldn’t make any user-facing changes, but it changes the balance in the bookmark list to address. Probably the issue label should be updated and the description explain it.
  • Previously we were storing publicKey of accounts to be ready for the new address system. Now only address is stored, which means that once lisk core team implements the new address system, all users of Hub will lose all bookmarks. I would like @reyraa to confirm if this is ok.
@massao

This comment has been minimized.

Copy link
Contributor Author

commented May 13, 2019

  • Previously we were storing publicKey of accounts to be ready for the new address system. Now only address is stored, which means that once lisk core team implements the new address system, all users of Hub will lose all bookmarks. I would like @reyraa to confirm if this is ok.

This one is my bad, I ended removing it, because it was not being used anywhere, but since it's to be used in the future, I'll put it back.

About the other two points, I think that @reyraa or @yasharAyari could update the ticket.

@massao massao force-pushed the 1962-implement-btc-bookmarks branch from ac8abc8 to 40a8716 May 13, 2019

@slaweet
Copy link
Member

left a comment

Works well 👍 Good job Massao.

@massao massao merged commit 0132e60 into development May 14, 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

@massao massao deleted the 1962-implement-btc-bookmarks branch May 14, 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.