Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Move Signer balance queries to store for component-wide re-use #3531

Merged
merged 8 commits into from
Nov 23, 2016

Conversation

jacogr
Copy link
Contributor

@jacogr jacogr commented Nov 18, 2016

Signer additions, 2 of n... Follow practices elsewhere where APIs are localised to stores, DRY-ing up queries, all within a single store.

  • Move balance queries to store, components are now not using api calls internally
  • Rename Request{Pending,Finished}Web3 -> Request{Pending,Finished} to actually indicate use-case + web3 has been removed pre 1.4

Closes https://github.com/ethcore/parity/issues/3523

@jacogr jacogr added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. M7-ui labels Nov 18, 2016
@jacogr jacogr added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Nov 22, 2016
@derhuerst
Copy link
Contributor

Although it's only a small part of cleaning up the signer, it looks good! 👍

@derhuerst derhuerst added A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. and removed A0-pleasereview 🤓 Pull request needs code review. labels Nov 22, 2016
@jacogr jacogr added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. labels Nov 22, 2016
@jacogr jacogr added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Nov 22, 2016
}

fetchBalances (addresses) {
addresses.forEach((address) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use of MobX transaction ?

Copy link
Contributor

Choose a reason for hiding this comment

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

NM, transaction isn't for async...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, as soon as you bring a promise in the transaction doesn't quite work - can only use it where setting the values, not wrapping a Promise. (Tried the former in my first attempt, however useStrict doesn't like that approach at all.)

@ngotchac ngotchac added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Nov 23, 2016
@gavofyork gavofyork merged commit 7ed4a21 into master Nov 23, 2016
@gavofyork gavofyork deleted the jg-signer-api-queries-2 branch November 23, 2016 15:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants