refactor price.js #356
refactor price.js #356
Conversation
✅ Deploy Preview for cheery-moxie-4f1121 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Generally I like the decoupling of modules, and this implementation is good.
I am unsure of what we should do to allow Oracles to be chosen via the Settings though, because this implementation differs from how cNetwork and cNode work, when they're all functionally the same: different endpoints.
| await this.getCurrencies(); | ||
| if (!this.#fLoadedCurrencies) await sleep(5000); | ||
| } | ||
| getEventEmitter().emit('currency-loaded', this.mapCurrencies); |
There was a problem hiding this comment.
| getEventEmitter().emit('currency-loaded', this.mapCurrencies); | |
| // Update any listeners for the full currency list (Settings, etc) | |
| getEventEmitter().emit('currency-loaded', this.mapCurrencies); | |
| // Update the balance to render the price instantly | |
| getEventEmitter().emit('balance-update'); |
Small suggestion: even after load() is successful after multiple failed attempts, it takes another >15s before balance-update is called by global.js, adding a delay to the currency displaying in the Dashboard, we can render both the Balances and Settings here.
| * @type {Oracle} | ||
| */ | ||
| export let cOracle = new Oracle(); | ||
|
|
There was a problem hiding this comment.
If we're removing the Oracle instantiation from here, we should at least add some method or settings variable for us to "change" between instances in real-time via Settings in the future, as this is not intended to be a hardcoded single class, it should function similar to cExplorer or cNode, which are both stored in settings.js.
There was a problem hiding this comment.
Having a setting to choose where you want to get the price from maybe is a bit too much...
Regardless we can do this change when we will actually implement the feature?
There was a problem hiding this comment.
I think users should have full control over their data 🤷♂️
And sure, we can save it for when more are hosted.
* refactor: create the event currency-update * refactor: move cOracle global variable to prices.js * load Oracle in constructor * Make fLoadedCurrencies a private member of the class * load the oracle on start * apply review * remove unused import
Abstract
A bit of refactor for the price class and its relation with settings:
fillCurrencySelectonly oncefLoadedCurrenciesprivateTesting
Test that price is still working as before