-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add AggregatorRouter Contract use case #8
Conversation
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.
LGTM! I left couple comments :)
package.json
Outdated
"typescript": "^4.9.4" | ||
} | ||
"name": "data-feed-consumer", | ||
"version": "0.2.0", |
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 think we should also increase version here.
@@ -7,15 +7,17 @@ import dotenv from 'dotenv' | |||
|
|||
dotenv.config() | |||
|
|||
let commonConfig = { gas: 5_000_000, gasPrice: 250_000_000_000 } | |||
let commonConfig = {} |
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.
Is there a reason to move definition inside the conditional branch?
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.
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.
It complains about accounts
, not about gas
and gasPrice
. Did you try to define accounts as below?
let commonConfig = { gas: 5_000_000, gasPrice: 250_000_000_000, accounts: [] }
Then we could just update the accounts
commonConfig.accounts = [process.env.PRIV_KEY]
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.
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.
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.
You don't have to waste time on it. We can fix it later ;)
contracts/DataFeedConsumer.sol
Outdated
int256 public answer; | ||
uint80 public roundId; | ||
|
||
constructor(address aggregatorProxy) { | ||
constructor(address aggregatorProxy, address aggregatorRouter) { |
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 think passing AggregatorProxy
loses its purpose after having access to the AggregatorRouter
. How having two consumer contracts? One showing how to use AggregatorProxy
and one for AggregatorRouter
?
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 felt bothered to make another contract and make separate deploy script, and thought that this is enough explanation for other developers to understand. But I'll write another contract if you insist
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.
Understood! We should assume that some people might want to take our code and make minimal modifications. In this case, they have to understand both approaches and always remove one of them. Please separate those two approaches into two consumer contracts.
contracts/DataFeedConsumer.sol
Outdated
, /* uint startedAt */ | ||
, /* uint updatedAt */ | ||
, /* uint80 answeredInRound */ | ||
int256 answer_ /* uint startedAt */ /* uint updatedAt */ /* uint80 answeredInRound */, |
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.
Can we keep the previous format? 🙏
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.
it's automatically formatted from my vs code... I'll turn off format on save option and update it
No description provided.