-
Notifications
You must be signed in to change notification settings - Fork 0
Removes dappId from adapter contracts #40
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
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
revert("Mock: Not implemented"); | ||
} | ||
|
||
function dappId() external pure override returns (uint256) { |
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.
Mhh, it's strange to see it implemented like this 🤔 Why not omit?
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.
We can't omit it because it is required by the IApi3ReaderProxyV1
interface but think of it as a safety measure where reverting immediately flags any unexpected use of the function during testing, which prevents bugs that might otherwise go unnoticed if we instead return some dummy value
|
||
let dappId2; | ||
if (!isLocalNetwork) { | ||
let dappId1, dappId2; |
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.
Why do we use the !isLocalNetwork
above?
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's no need to check for dappId equality locally (NETWORK=hardhat) because the script uses mock proxies for testing
4bb16bb
to
6ea628c
Compare
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
6ea628c
to
154359b
Compare
Closes #39
Implementation details
dappId
from adapter contractsIApi3ReaderProxy
compliantIApi3ReaderProxyV1
compliant and dappIds don't matchNow we should be able to deploy a ProductApi3ReaderProxyV1 composing wstETH exchange rate with ETH/USD feed since
dappId
will no longer be checked onchain nor during deployment but in cases where both proxies have dappIds then the deploy script will make sure they are equal.