Smarter Tokens fetching #3546

ngotchac · 2016-11-21T11:43:00Z

WIP for #3240

It was useless to fetch data for each and every token on every new block.
Tokens data is not editable. Only meta data can be modified, so we just need to check for this specific event on new block only (no need to poll every second). Same for new tokens registered (might not even be that necessary...).

Fixes also an issue where all accounts were being checked for balances, even deleted ones.

coveralls · 2016-11-21T12:15:09Z

Changes Unknown when pulling 54e2844 on ng-smarter-contracts into ** on master**.

jacogr · 2016-11-21T14:04:30Z

js/src/api/format/input.js

@@ -50,10 +51,15 @@ export function inHash (hash) {
  return inHex(hash);
 }

+export function pad (input, length) {


Don't we have this function already? (Seems duplicated)

This pads the topics on the right

jacogr · 2016-11-21T14:06:48Z

js/src/api/contract/contract.js

@@ -232,51 +233,102 @@ export default class Contract {
      return this._subscribe(event, options, callback);
    };

+    event.register = (options = {}, callback) => {


Ok, not 100% sure I get the difference between event.register and event.subscribe - not from an API user POV.

The register method actually calls eth_getFilterChanges every second, which is unnecessary for getting token meta data updates (will be max a change for every block). We could actually call it every minute or so if needed...

Ok, still unsure about how to explain this since it sits on the API interface. Obviously for any filter when pending is not included (toBlock) just calling the new filter on new blocks is more than enough. (Something to probably think about and enhance)

The case here is that we have 2 external APIs now - how does anybody decide what to use, seems we are making the external interface very complex now.

I guess for filters querying pending blocks too it could be useful to query every second (not sure though). True from the API perspective, but didn't want to change the whole behavior.

Agreed. As a dapp developer - what do I use? subscribe or register? As it stands the naming is almost interchangeable. We want things simple so people don't need to overthink things, but still allow them to do anything.

So to accomplish this, would have -

not have yet another external interface event

only polled for rapid changes when pending is the toBlock

poll for filter changes on new blocks only when latest is toBlock

when we poll for tokens, specify latest

... all wrapped in contracts.js. This way anybody using the API doesn't need to care about times and polling, the API just does the right thing, as asked. (Plus no confusion)

I agree with this. First I wanted to call it manualSubscribe or something like that. Will fix

derhuerst · 2016-11-21T17:21:53Z

js/src/api/contract/contract.js

+    this._sendSubscriptionChanges();
+  }
+
+  _unsubscribeToChanges = () => {


unsubscribeFromChanges? (;

derhuerst · 2016-11-21T17:37:49Z

I haven't looked into this in detail, but at ng-smarter-contracts the events of my account 0x00D189b71E5b42a88aa3e83173D4a6926e665336 are not showing up, while on master, they are.

ngotchac · 2016-11-21T18:15:35Z

The transactions ?

derhuerst · 2016-11-21T18:31:49Z

@ngotchac

The transactions ?

Yes, may be related to master commits not merged into ng-smarter-contracts.

coveralls · 2016-11-21T18:48:30Z

Changes Unknown when pulling ef2c170 on ng-smarter-contracts into ** on master**.

jacogr · 2016-11-21T19:58:17Z

js/src/redux/providers/balances.js

+        toBlock: 'latest'
+      }, (error, logs) => {
+        if (error) {
+          return console.error('balances::attachToNewToken', 'failed to attacht to tokenreg Registered', error.toString(), error.stack);


s/attacht/attach/

coveralls · 2016-11-22T10:32:28Z

Changes Unknown when pulling 7f36600 on ng-smarter-contracts into ** on master**.

jacogr · 2016-11-22T15:33:27Z

Failing tests -


  1) api/contract/Contract subscribe valid events sets the subscriptionId returned:
     TransportError: eth_newFilter: undefined: undefined
      at http.js:77:25
      at process._tickCallback (internal/process/next_tick.js:103:7)

  2) api/contract/Contract subscribe valid events creates a new filter and retrieves the logs on it:
     TransportError: eth_newFilter: undefined: undefined
      at http.js:77:25
      at process._tickCallback (internal/process/next_tick.js:103:7)

  3) api/contract/Contract subscribe valid events returns the logs to the callback:
     TransportError: eth_newFilter: undefined: undefined
      at http.js:77:25
      at process._tickCallback (internal/process/next_tick.js:103:7)

coveralls · 2016-11-22T18:26:24Z

Changes Unknown when pulling f4f2c5e on ng-smarter-contracts into ** on master**.

ngotchac added 8 commits November 18, 2016 18:49

Don't auto-subscribe for contracts #3240

0e53f7d

Smarter Balance : don't re-instantiate contracts, fetch tokens #3240

601e9ec

Smarter Balance Tokens fetching (update image when needed) #3240

9ebe84a

Attaching to TokenReg Events instead of fetching for each block #3240

3f332a0

Merge branch 'master' into ng-smarter-contracts

34524e9

Unsubscribe from shapeshit... #3240

7db0ad1

Unsubscribe from EthFilter if used once (resolved) #3240

2ec182a

Remove warning

54e2844

ngotchac added A0-pleasereview 🤓 Pull request needs code review. M7-ui labels Nov 21, 2016

jacogr reviewed Nov 21, 2016

View reviewed changes

derhuerst reviewed Nov 21, 2016

View reviewed changes

PR review fix

ef2c170

jacogr reviewed Nov 21, 2016

View reviewed changes

ngotchac added 3 commits November 22, 2016 09:44

Typo

5b5dc9b

Merge branch 'master' into ng-smarter-contracts

dc69e5d

Better contract JS API : one subscribe for all #3546

7f36600

jacogr 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

Fixed test

f4f2c5e

jacogr added the A8-looksgood 🦄 Pull request is reviewed well. label Nov 23, 2016

jacogr removed the A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. label Nov 23, 2016

jacogr merged commit 33dd491 into master Nov 23, 2016

jacogr deleted the ng-smarter-contracts branch November 23, 2016 09:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Smarter Tokens fetching #3546

Smarter Tokens fetching #3546

ngotchac commented Nov 21, 2016

coveralls commented Nov 21, 2016

jacogr Nov 21, 2016

ngotchac Nov 22, 2016

jacogr Nov 21, 2016

ngotchac Nov 21, 2016

jacogr Nov 21, 2016

ngotchac Nov 21, 2016

jacogr Nov 21, 2016

ngotchac Nov 22, 2016 •

edited

Loading

derhuerst Nov 21, 2016

derhuerst commented Nov 21, 2016

ngotchac commented Nov 21, 2016

derhuerst commented Nov 21, 2016

coveralls commented Nov 21, 2016

jacogr Nov 21, 2016

coveralls commented Nov 22, 2016

jacogr commented Nov 22, 2016

coveralls commented Nov 22, 2016

Smarter Tokens fetching #3546

Smarter Tokens fetching #3546

Conversation

ngotchac commented Nov 21, 2016

coveralls commented Nov 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngotchac Nov 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derhuerst commented Nov 21, 2016

ngotchac commented Nov 21, 2016

derhuerst commented Nov 21, 2016

coveralls commented Nov 21, 2016

Choose a reason for hiding this comment

coveralls commented Nov 22, 2016

jacogr commented Nov 22, 2016

coveralls commented Nov 22, 2016

ngotchac Nov 22, 2016 •

edited

Loading