[price-pusher] add injective price listener #627
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ |
|
|
||
| const argv = yargs(hideBin(process.argv)) | ||
| .option("evm-endpoint", { | ||
| .option("network", { |
There was a problem hiding this comment.
Can you also update the readme with this new configs and values? You would need to update the compose files too.
There was a problem hiding this comment.
I am not sure if this file is final yet.
I have added the code to it just so that injective pieces are working fine.
I will refactor this file too and then will update the README
There was a problem hiding this comment.
command line options will be annoying as the configuration becomes more complicated. I suggest moving this stuff to a config file. I note that there's already a price config file, so maybe it goes in there.
| "jest": "^27.5.1", | ||
| "prettier": "^2.6.2", | ||
| "ts-jest": "^27.1.4", | ||
| "typescript": "^4.6.3" |
There was a problem hiding this comment.
I think we need to bump version as major for this change.
There was a problem hiding this comment.
will send a pr with readme and version changes. which will be the final one after refactoring.
| const priceConfigs = readPriceConfigFile(argv.priceConfigFile); | ||
|
|
||
| async function run() { | ||
| async function injectiveRun() { |
There was a problem hiding this comment.
minor: I think injective and evm can have the same run function (or use a common function for running controller, ...) as many things are similar in your code.
There was a problem hiding this comment.
as mentioned above
will clean this file in separate PR
There was a problem hiding this comment.
FYI if you want to do this stuff in a separate PR, i suggest leaving a code comment like
// FIXME: unify with evmRun() below once the interfaces are figured out.
then people will understand what you're trying to do when reading the PR and won't leave comments about it
| }; | ||
|
|
||
| // this use price without leading 0x | ||
| export class InjectivePriceListener implements PriceListener { |
There was a problem hiding this comment.
I see that this is very similar to EVM. Maybe it's good to extract common identical methods (that will be the same for other networks too) to PriceListener.
There was a problem hiding this comment.
agree with this. i think the latestPriceInfo / update / get methods could be moved up to priceListener
I wouldn't move the pollPrices method even though it is very similar to the evm method, as you want to give implementations the flexibility to implement the reading logic in different ways.
ali-behjati
left a comment
ali-behjati
left a comment
There was a problem hiding this comment.
This is awesome. I really appreciate that you are improving the quality of this code. I left some comments and will approve once you address them.
jayantk
left a comment
jayantk
left a comment
There was a problem hiding this comment.
i think extracting common methods like @ali-bahjati says is a good thing to do. If ali is happy, then I am; no need to wait for me to approve.
| }; | ||
|
|
||
| // this use price without leading 0x | ||
| export class InjectivePriceListener implements PriceListener { |
There was a problem hiding this comment.
agree with this. i think the latestPriceInfo / update / get methods could be moved up to priceListener
I wouldn't move the pollPrices method even though it is very similar to the evm method, as you want to give implementations the flexibility to implement the reading logic in different ways.
| const priceConfigs = readPriceConfigFile(argv.priceConfigFile); | ||
|
|
||
| async function run() { | ||
| async function injectiveRun() { |
There was a problem hiding this comment.
FYI if you want to do this stuff in a separate PR, i suggest leaving a code comment like
// FIXME: unify with evmRun() below once the interfaces are figured out.
then people will understand what you're trying to do when reading the PR and won't leave comments about it
No description provided.