-
Notifications
You must be signed in to change notification settings - Fork 54
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
[feat] CCIP capability config contract #858
Conversation
I see you updated files related to
|
I see you updated files related to |
LCOV of commit
|
the following funcs are left: * getAllOCRConfigs * getAllChainConfigs * getCapabilityConfiguration * beforeCapabilityConfigSet * _updatePluginConfig
a2449dc
to
a15d291
Compare
abd69e2
to
049cc03
Compare
51c50fa
to
ef97195
Compare
contracts/src/v0.8/ccip/capability/CCIPCapabilityConfiguration.sol
Outdated
Show resolved
Hide resolved
// TODO: check for duplicate signers, duplicate p2p ids, etc. | ||
// TODO: check that p2p ids in cfg.bootstrapP2PIds are included in cfg.p2pIds. | ||
for (uint256 i = 0; i < cfg.signers.length; ++i) { | ||
_ensureInRegistry(cfg.p2pIds[i]); |
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 very expensive to do an external call for each signer
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.
Do we need this validation at all? If the Capability Registry changes the p2p IDs, wouldn't this contract go out of sync w/ the registry?
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.
The idea behind this validation is to ensure that the p2p id we're setting in the ocr config is present in the registry, since the registry doesn't do that check (it just sees opaque bytes).
After discussing offline it seems like this will cost around 25k (+/- some stuff) gas for 31 oracles which is the worst case. Its not great but can be improved. Some suggestions:
- Doing a nested for loop in my forge tests is less gassy by like 5k
- Pre-sorting both lists of p2p ids given to the capability registry and in the ocr config, and doing a sorted comparison
Will create a ticket to address this after, seems like a good gas optimization task
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.
Created CCIP-2613 to address this
Motivation
The CCIP capability needs to fetch its configuration from somewhere - that somewhere is the CCIP capability configuration contract.
Solution
Implement the CCIP capability configuration contract in solidity. The solidity implementation almost matches the design doc exactly, except for getAllOCRConfigs() which I haven't implemented yet until we figure out the best function signature for it: