This repository has been archived by the owner on Nov 6, 2020. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Smarter Tokens fetching #3546
Merged
Merged
Smarter Tokens fetching #3546
Changes from 8 commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
0e53f7d
Don't auto-subscribe for contracts #3240
ngotchac 601e9ec
Smarter Balance : don't re-instantiate contracts, fetch tokens #3240
ngotchac 9ebe84a
Smarter Balance Tokens fetching (update image when needed) #3240
ngotchac 3f332a0
Attaching to TokenReg Events instead of fetching for each block #3240
ngotchac 34524e9
Merge branch 'master' into ng-smarter-contracts
ngotchac 7db0ad1
Unsubscribe from shapeshit... #3240
ngotchac 2ec182a
Unsubscribe from EthFilter if used once (resolved) #3240
ngotchac 54e2844
Remove warning
ngotchac ef2c170
PR review fix
ngotchac 5b5dc9b
Typo
ngotchac dc69e5d
Merge branch 'master' into ng-smarter-contracts
ngotchac 7f36600
Better contract JS API : one subscribe for all #3546
ngotchac f4f2c5e
Fixed test
ngotchac File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -48,7 +48,8 @@ export default class Contract { | |
this._instance[fn.signature] = fn; | ||
}); | ||
|
||
this._sendSubscriptionChanges(); | ||
this._subscribedToChanges = false; | ||
this._subscribedToChangesId = null; | ||
} | ||
|
||
get address () { | ||
|
@@ -232,51 +233,102 @@ export default class Contract { | |
return this._subscribe(event, options, callback); | ||
}; | ||
|
||
event.register = (options = {}, callback) => { | ||
return this._register(event, options, callback); | ||
}; | ||
|
||
event.unsubscribe = (subscriptionId) => { | ||
return this.unsubscribe(subscriptionId); | ||
}; | ||
|
||
return event; | ||
} | ||
|
||
subscribe (eventName = null, options = {}, callback) { | ||
return new Promise((resolve, reject) => { | ||
let event = null; | ||
_register (event = null, _options, callback) { | ||
return this | ||
._createEthFilter(event, _options) | ||
.then((filterId) => { | ||
return { | ||
fetch: () => { | ||
return this._api.eth | ||
.getFilterChanges(filterId) | ||
.then((logs) => { | ||
if (!logs || !logs.length) { | ||
return; | ||
} | ||
|
||
try { | ||
callback(null, this.parseEventLogs(logs)); | ||
} catch (error) { | ||
callback(error); | ||
} | ||
}); | ||
}, | ||
|
||
unsubscribe: () => { | ||
return this._api.eth.uninstallFilter(filterId); | ||
} | ||
}; | ||
}); | ||
} | ||
|
||
if (eventName) { | ||
event = this._events.find((evt) => evt.name === eventName); | ||
_findEvent (eventName = null) { | ||
const event = eventName | ||
? this._events.find((evt) => evt.name === eventName) | ||
: null; | ||
|
||
if (!event) { | ||
const events = this._events.map((evt) => evt.name).join(', '); | ||
reject(new Error(`${eventName} is not a valid eventName, subscribe using one of ${events} (or null to include all)`)); | ||
return; | ||
} | ||
} | ||
if (eventName && !event) { | ||
const events = this._events.map((evt) => evt.name).join(', '); | ||
throw new Error(`${eventName} is not a valid eventName, subscribe using one of ${events} (or null to include all)`); | ||
} | ||
|
||
return this._subscribe(event, options, callback).then(resolve).catch(reject); | ||
}); | ||
return event; | ||
} | ||
|
||
_subscribe (event = null, _options, callback) { | ||
const subscriptionId = nextSubscriptionId++; | ||
_createEthFilter (event = null, _options) { | ||
const optionTopics = _options.topics || []; | ||
const signature = event && event.signature || null; | ||
|
||
// If event provided, remove the potential event signature | ||
// as the first element of the topics | ||
const topics = signature | ||
? [ signature ].concat(optionTopics.filter((t, idx) => idx > 0 || t !== signature)) | ||
: optionTopics; | ||
|
||
const options = Object.assign({}, _options, { | ||
address: this._address, | ||
topics: [event ? event.signature : null] | ||
topics | ||
}); | ||
|
||
return this._api.eth | ||
.newFilter(options) | ||
return this._api.eth.newFilter(options); | ||
} | ||
|
||
subscribe (eventName = null, options = {}, callback) { | ||
try { | ||
const event = this._findEvent(eventName); | ||
return this._subscribe(event, options, callback); | ||
} catch (e) { | ||
return Promise.reject(e); | ||
} | ||
} | ||
|
||
_subscribe (event = null, _options, callback) { | ||
const subscriptionId = nextSubscriptionId++; | ||
|
||
return this | ||
._createEthFilter(event, _options) | ||
.then((filterId) => { | ||
return this._api.eth | ||
.getFilterLogs(filterId) | ||
.then((logs) => { | ||
callback(null, this.parseEventLogs(logs)); | ||
|
||
this._subscriptions[subscriptionId] = { | ||
options, | ||
callback, | ||
filterId | ||
}; | ||
|
||
this._subscribeToChanges(); | ||
return subscriptionId; | ||
}); | ||
}); | ||
|
@@ -287,12 +339,30 @@ export default class Contract { | |
.uninstallFilter(this._subscriptions[subscriptionId].filterId) | ||
.then(() => { | ||
delete this._subscriptions[subscriptionId]; | ||
|
||
if (Object.keys(this._subscriptions).length === 0) { | ||
this._unsubscribeToChanges(); | ||
} | ||
}) | ||
.catch((error) => { | ||
console.error('unsubscribe', error); | ||
}); | ||
} | ||
|
||
_subscribeToChanges = () => { | ||
if (this._subscribedToChanges) { | ||
return; | ||
} | ||
|
||
this._subscribedToChanges = true; | ||
this._sendSubscriptionChanges(); | ||
} | ||
|
||
_unsubscribeToChanges = () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
this._subscribedToChanges = false; | ||
clearTimeout(this._subscribedToChangesId); | ||
} | ||
|
||
_sendSubscriptionChanges = () => { | ||
const subscriptions = Object.values(this._subscriptions); | ||
const timeout = () => setTimeout(this._sendSubscriptionChanges, 1000); | ||
|
@@ -316,11 +386,11 @@ export default class Contract { | |
} | ||
}); | ||
|
||
timeout(); | ||
this._subscribedToChangesId = timeout(); | ||
}) | ||
.catch((error) => { | ||
console.error('_sendSubscriptionChanges', error); | ||
timeout(); | ||
this._subscribedToChangesId = timeout(); | ||
}); | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,6 +15,7 @@ | |
// along with Parity. If not, see <http://www.gnu.org/licenses/>. | ||
|
||
import BigNumber from 'bignumber.js'; | ||
import { range } from 'lodash'; | ||
|
||
import { isArray, isHex, isInstanceOf, isString } from '../util/types'; | ||
|
||
|
@@ -50,10 +51,15 @@ export function inHash (hash) { | |
return inHex(hash); | ||
} | ||
|
||
export function pad (input, length) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't we have this function already? (Seems duplicated) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This pads the topics on the right |
||
const value = inHex(input).substr(2, length * 2); | ||
return '0x' + value + range(length * 2 - value.length).map(() => '0').join(''); | ||
} | ||
|
||
export function inTopics (_topics) { | ||
let topics = (_topics || []) | ||
.filter((topic) => topic) | ||
.map(inHex); | ||
.filter((topic) => topic === null || topic) | ||
.map((topic) => topic === null ? null : pad(topic, 32)); | ||
|
||
while (topics.length < 4) { | ||
topics.push(null); | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, not 100% sure I get the difference between
event.register
andevent.subscribe
- not from an API user POV.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
register
method actually callseth_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...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. As a dapp developer - what do I use?
subscribe
orregister
? 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 -
pending
is thetoBlock
latest
istoBlock
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)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with this. First I wanted to call it
manualSubscribe
or something like that. Will fix