-
Notifications
You must be signed in to change notification settings - Fork 0
Adds dappId
to combinator contracts
#29
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
e37a8aa
to
0beed2b
Compare
@acenolaza I'll be able to review this only after my vacation, but I've assigned the other guys for the preliminary review. |
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've checked the contract changes (didn't go over the tests) and IMO they're looking fine.
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.
Changes LGTM, but I would also see your suggestion as a good thing to do.
I think it might be more accurate to create a IApi3ReaderProxyWithDappId or similar that inherints from IApi3ReaderProxyand adds a function dappId() external view returns (uint256);
0beed2b
to
59a38ef
Compare
abb3cf3 this should improve the cast of the proxies to a valid minimal interface that adheres to only what is needed on these combinator contracts. |
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'm thinking whether it could be simplified if we used the IApi3ReaderProxyV1
instead of the new interface, but I'm thinking if there was another reason why we didn't do it like that initially.
abb3cf3
to
d6abe74
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
Closes #24
Changes
dappId
immutable variable to all combinator contracts. These are set by reading the underlying proxy.dappId() (except for NormalizedApi3ReaderProxyV1)IApi3ReaderProxyV1
NormalizedApi3ReaderProxyV1
now requires adappId
to be pass in the constructorProductApi3ReaderProxyV1
MockApi3ReaderProxyV1
test contract to make testing simplerNotes
dappId()
inIApi3ReaderProxyV1
is not marked asview
IApi3ReaderProxyV1
still feels kind of wrong to me since the underlying proxies might be another combinator proxy which might not even expose adapiName()
function for example. I think it might be more accurate to create aIApi3ReaderProxyWithDappId
or similar that inherints fromIApi3ReaderProxy
and adds a functiondappId() external view returns (uint256);