Implement CCIPHome based on RMNHome #1459
Conversation
LCOV of commit
|
There was a problem hiding this comment.
At a high level, I feel like HomeBase might be trying to do too much. I think allowing a little bit of duplication b/t CCIPHome and RMNHome might bring down the overall complexity.
Ex could we allow both CCIPHome and RMNHome to define their own config storage and validation? That would allow us to remove the _validate...Config() callbacks. Doing that would also allow the setConfig functions to be strongly typed (instead of doing abi-decode on chain) which, which tends to be simpler for off chain to deal with.
We could still use HomeBase for revoking & promoting. Maybe versioning too?
|
|
||
| /// @notice Called by the registry prior to the config being set for a particular DON. | ||
| /// @dev precondition Requires destination chain config to be set | ||
| function beforeCapabilityConfigSet( |
| function getConfig( | ||
| bytes32 pluginKey, | ||
| bytes32 configDigest | ||
| ) external view returns (VersionedConfig memory versionedConfig, bool ok) { | ||
| (StoredConfig memory storedConfig, bool configOK) = _getStoredConfig(pluginKey, configDigest); | ||
| if (configOK) { | ||
| return ( | ||
| VersionedConfig({ | ||
| version: storedConfig.version, | ||
| configDigest: storedConfig.configDigest, | ||
| staticConfig: abi.decode(storedConfig.staticConfig, (OCR3Config)) | ||
| }), | ||
| true | ||
| ); | ||
| } | ||
|
|
||
| return (versionedConfig, false); | ||
| } | ||
|
|
||
| function getAllConfigs( |
There was a problem hiding this comment.
suggestion: I find it a touch incongruent that getConfig() returns a valid/invalid boolean, while getAllConfigs() just returns empty structs if the configs are unset. I understand why _getStoredConfig() returns the bool so that the parent contract can decide how to handle such cases. In getConfig() I think it might be more consistent to just return empty.
| // │ Validation │ | ||
| // ================================================================ | ||
|
|
||
| function _validateStaticAndDynamicConfig(bytes memory encodedStaticConfig, bytes memory) internal view override { |
There was a problem hiding this comment.
is there no dynamic config?
There was a problem hiding this comment.
CCIP does not use config that is not part of the config digest (dynamic config) at this time
| bytes calldata encodedStaticConfig, | ||
| bytes calldata encodedDynamicConfig, |
There was a problem hiding this comment.
I usually try to avoid defining external function where data has to be pre-encoded in order to call the func. I know we're trying to make things generic here, but I would opt for making this an internal function and then allowing CCIPHome and RMNHome to call this function after abi.encoding their respective config structs.
There was a problem hiding this comment.
That would significantly add to the cost right? Is that worth it?
There was a problem hiding this comment.
Hmm I think you'd avoid the abi.decode in the validation hook that way, because you could validate before calling setSecondary. So I think similar gas wise but definitely more readable without the validation hook?
|
RensR
force-pushed
the
ccipHome-with-rmn-home
branch
from
338d552
to
68b96a7
Compare
| // 1. promoteSecondaryAndRevokePrimary requires | ||
| // - blue digest to be the current secondary digest | ||
| // - green digest to be the zero digest | ||
| if (currentGreenDigest == newBlueDigest && newGreenDigest == ZERO_DIGEST) { |
There was a problem hiding this comment.
hm won't newBlueDigest never be equal to currentGreenDigest because the versions are different? E.g.
- Say count=1 and blue=nil, green=C (initial set). Digest of green is hash(.., C, 1)
- Now set blue=C, green=nil.
- newConfigVersion=2, hash(..,C,2) != hash(..,C,1)?
There was a problem hiding this comment.
ah maybe the fix is straightforward just need newBlueDigest to be newBlueDigestWithCurrentVersion
There was a problem hiding this comment.
In the current design the versions don't change when you promote a config, I thought that was the desired effect?
RensR
force-pushed
the
ccipHome-with-rmn-home
branch
from
ba42282
to
60d07b4
Compare
No description provided.