feat: add getter to return all source chain config #1477
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: 0xsuryansh <[email protected]>
LCOV of commit
|
Signed-off-by: 0xsuryansh <[email protected]> feat: add getter to return all source chain config Signed-off-by: 0xsuryansh <[email protected]>
Signed-off-by: 0xsuryansh <[email protected]>
Signed-off-by: 0xsuryansh <[email protected]>
0xsuryansh
requested review from
makramkd,
elatoskinas,
RayXpub and
a team
as code owners
Signed-off-by: 0xsuryansh <[email protected]>
RensR
reviewed
Oct 2, 2024
| @@ -919,6 +924,16 @@ contract OffRamp is ITypeAndVersion, MultiOCR3Base { | |||
| return s_sourceChainConfigs[sourceChainSelector]; | |||
| } | |||
|
|
|||
| /// @notice Returns all source chain configs | |||
| /// @return sourceChainConfigs The source chain configs corresponding to all the supported chain selectors | |||
| function getAllSourceChainConfig() external view returns (SourceChainConfig[] memory) { | |||
There was a problem hiding this comment.
This doesn't return the chain selectors, which I would suspect are required?
RensR
reviewed
Oct 2, 2024
| @@ -931,6 +946,7 @@ contract OffRamp is ITypeAndVersion, MultiOCR3Base { | |||
| for (uint256 i = 0; i < sourceChainConfigUpdates.length; ++i) { | |||
| SourceChainConfigArgs memory sourceConfigUpdate = sourceChainConfigUpdates[i]; | |||
| uint64 sourceChainSelector = sourceConfigUpdate.sourceChainSelector; | |||
| s_sourceChainSelectors.add(sourceChainSelector); | |||
There was a problem hiding this comment.
Lets do this after the sanity checks
Signed-off-by: 0xsuryansh <[email protected]>
Signed-off-by: 0xsuryansh <[email protected]>
makramkd
reviewed
Oct 3, 2024
| @@ -919,6 +924,18 @@ contract OffRamp is ITypeAndVersion, MultiOCR3Base { | |||
| return s_sourceChainConfigs[sourceChainSelector]; | |||
| } | |||
|
|
|||
| /// @notice Returns all source chain configs | |||
| /// @return sourceChainConfigs The source chain configs corresponding to all the supported chain selectors | |||
| function getAllSourceChainConfig() external view returns (uint64[] memory, SourceChainConfig[] memory) { | |||
There was a problem hiding this comment.
naming nit: getAllSourceChainConfigs (with an s at the end)? Whats the preference for this @RensR
There was a problem hiding this comment.
Its a list so yeah good call
There was a problem hiding this comment.
RensR
reviewed
Oct 3, 2024
| @@ -962,6 +979,8 @@ contract OffRamp is ITypeAndVersion, MultiOCR3Base { | |||
| currentConfig.isEnabled = sourceConfigUpdate.isEnabled; | |||
| currentConfig.router = sourceConfigUpdate.router; | |||
|
|
|||
| s_sourceChainSelectors.add(sourceChainSelector); | |||
There was a problem hiding this comment.
nit: this returns a bool for "inserted" or "already exists". maybe add a comment explaining we don't care about the return value as adding it twice should not do anything
There was a problem hiding this comment.
Signed-off-by: 0xsuryansh <[email protected]>
# Conflicts: # contracts/gas-snapshots/ccip.gas-snapshot # core/gethwrappers/ccip/generated/offramp/offramp.go # core/gethwrappers/ccip/generation/generated-wrapper-dependency-versions-do-not-edit.txt
Signed-off-by: 0xsuryansh <[email protected]>
makramkd
approved these changes
Oct 4, 2024
RensR
approved these changes
Oct 7, 2024
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
The current offchain initialization process involves calling getSourceChainConfig(sourceChainSelector), which retrieves configurations for a specific source chain selector. However, there's no mechanism to fetch a complete list of supported source chains during the initialization phase. This limitation complicates the process, as the full list of source chain selectors is not readily available.
Solution
To address this, the proposed solution introduces an enumerable set of all source chain selectors. By implementing a getAllSourceChainConfig() function, we can easily retrieve the complete list of source chains on-chain. This eliminates the need for additional rounds of OCR gossip to share known source chains, simplifying initialization and improving efficiency.