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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement minimal BTC wallet page - Closes #1888 #2003

Open
wants to merge 36 commits into
base: development
from

Conversation

Projects
None yet
3 participants
@slaweet
Copy link
Member

commented May 13, 2019

What issue have I solved?

#1888

How have I implemented/fixed it?

  • I updated walletTransactionsV2 HOC to get data based on activeToken.
  • I updated various components not to show if token is BTC

There are two refactoring changes not essential to the problem at hand

  • 馃敟 Remove functionMapper to make code cleaner d2f8280
  • 鈾伙笍 Move updateTransactionsIfNeeded to transactions actions 02b2ae4

How has this been tested?

  • Go to wallet
  • Open React dev tools (because of https://stackoverflow.com/a/44318447/2080716)
  • Enable BTC feature localStorage.setItem('btc', true);
  • Set BTC active token by running this in console $r.store.dispatch({type:'SETTINGS_UPDATE_TOKEN', data: { token: { active: 'BTC', list: {BTC: true, LSK: true} }}}) Note that React dev tools have to be active for $r to be defined.
  • Switch active token back to LSK by running this in console $r.store.dispatch({type:'SETTINGS_UPDATE_TOKEN', data: { token: { active: 'LSK', list: {BTC: true, LSK: true} }}}).

Review checklist

@slaweet slaweet self-assigned this May 13, 2019

@slaweet slaweet force-pushed the 1888-minimal-btc-wallet-page branch 2 times, most recently from 177fad1 to ad02b38 May 13, 2019

slaweet added some commits May 9, 2019

@slaweet slaweet force-pushed the 1888-minimal-btc-wallet-page branch from ad02b38 to 9a53600 May 13, 2019

slaweet added some commits May 13, 2019

馃尡 Hide incoming/outgoing filter for BTC tranasctions
because it's not yet implemented server-side

@slaweet slaweet marked this pull request as ready for review May 14, 2019

@slaweet slaweet requested a review from massao May 14, 2019

@massao
Copy link
Contributor

left a comment

Changing to BTC works properly, but when changing the token back to LSK, the application stops responding.

@slaweet slaweet requested a review from massao May 14, 2019

@slaweet slaweet force-pushed the 1888-minimal-btc-wallet-page branch from 043c5ca to fa5bf1d May 15, 2019

@slaweet slaweet force-pushed the 1888-minimal-btc-wallet-page branch from fa5bf1d to 1798514 May 15, 2019

@massao
Copy link
Contributor

left a comment

It's working nicely now, just need to check why the e2e are failing, maybe it's just flaky tests

馃悰 Prevent app stuck on token switch
The problem was in the balanceChart component. It tried to render BTC
transactions before the LSK transactions were fetched. The problem with
BTC transactions was that they have different timestamp and it broke the
whole app.

@slaweet slaweet force-pushed the 1888-minimal-btc-wallet-page branch from 1798514 to d8b5e98 May 15, 2019

@yasharAyari yasharAyari self-requested a review May 15, 2019

@yasharAyari
Copy link
Member

left a comment

Please review the transactions action file and try to remove and merge unnecessary actions inside that. Also, when the active token is BTC the application throws some errors. check the attached image for more information.
Screenshot 2019-05-15 at 11 04 28

}));
dispatch(passphraseUsed(passphrase));
}
};

export const updateTransactionsIfNeeded = ({ transactions, account }, windowFocus) =>

This comment has been minimized.

Copy link
@yasharAyari

yasharAyari May 15, 2019

Member

this method is completely unnecessary and confusing. you can use other methods instead of this.

This comment has been minimized.

Copy link
@slaweet

slaweet May 15, 2019

Author Member

I didn't create this function, I just moved it from src/actions/account.js when I was cleaning up that file a bit. I agree it is confusing, but I'm not sure if I can just remove it and not break anything. Either way, it's out of the scope of this pr.

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

This comment has been minimized.

Copy link
Member Author

commented May 15, 2019

@yasharAyari I solved some problem, now my console is clean:
Screenshot 2019-05-15 at 12 53 13

Please check again.

@slaweet slaweet requested a review from yasharAyari May 15, 2019

@slaweet slaweet requested a review from massao May 15, 2019

@slaweet slaweet removed request for massao and yasharAyari May 16, 2019

slaweet added some commits May 16, 2019

馃悰 Fix how new transactions appear
The problem was that a new transaction could have reused the DOM of an
exiting transactions because the key was the index. Now with key being
the transaction id, it correctly animates new pending transaction when
there are multiple pending transactions.
It also solves "Date invalid" problem.

@slaweet slaweet force-pushed the 1888-minimal-btc-wallet-page branch from 46b6f40 to 2f40746 May 16, 2019

slaweet added some commits May 17, 2019

馃悰 Remove undefined action type from transactionsListV2
so that it doesn't do false-positive loading

@slaweet slaweet force-pushed the 1888-minimal-btc-wallet-page branch from 24d216d to 4abaee9 May 17, 2019

@slaweet slaweet force-pushed the 1888-minimal-btc-wallet-page branch from 4abaee9 to 6565e1f May 17, 2019

slaweet added some commits May 17, 2019

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