Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[feat] CCIP capability config contract #858

Open
wants to merge 21 commits into
base: ccip-develop
Choose a base branch
from
Open

[feat] CCIP capability config contract #858

wants to merge 21 commits into from
+3,123 −0

Conversation

makramkd
Copy link
Contributor

@makramkd makramkd commented May 15, 2024 •
edited

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:

  • return all OCR configs for all plugins
  • take a donId, returns configs for all plugins
  • take a plugin type, return all configs (for all dons) for that plugin type
  • etc.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented May 15, 2024 •
edited

I see you updated files related to core. Please run pnpm changeset in the root directory to add a changeset as well as in the text include at least one of the following tags:

  • #added For any new functionality added.
  • #breaking_change For any functionality that requires manual action for the node to boot.
  • #bugfix For bug fixes.
  • #changed For any change to the existing functionality.
  • #db_update For any feature that introduces updates to database schema.
  • #deprecation_notice For any upcoming deprecation functionality.
  • #internal For changesets that need to be excluded from the final changelog.
  • #nops For any feature that is NOP facing and needs to be in the official Release Notes for the release.
  • #removed For any functionality/config that is removed.
  • #updated For any functionality that is updated.
  • #wip For any change that is not ready yet and external communication about it should be held off till it is feature complete.

@github-actions GitHub Actions
Copy link
Contributor

I see you updated files related to contracts. Please run pnpm changeset in the contracts directory to add a changeset.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented May 15, 2024 •
edited

LCOV of commit f102053 during Solidity Foundry #5001

Summary coverage rate:
  lines......: 98.6% (1605 of 1627 lines)
  functions..: 96.6% (316 of 327 functions)
  branches...: 91.7% (693 of 756 branches)

Files changed coverage rate: n/a

@makramkd
more tests
de4e4ce 
the following funcs are left:
* getAllOCRConfigs
* getAllChainConfigs
* getCapabilityConfiguration
* beforeCapabilityConfigSet
* _updatePluginConfig
@github-actions GitHub Actions
Copy link
Contributor

Go solidity wrappers are out-of-date, regenerate them via the make wrappers-all command

@makramkd makramkd marked this pull request as ready for review May 28, 2024 10:56
@makramkd makramkd requested review from a team as code owners May 28, 2024 10:56
elatoskinas
elatoskinas reviewed May 29, 2024
View reviewed changes
contracts/src/v0.8/ccip/capability/interfaces/ICapabilityRegistry.sol Outdated Show resolved Hide resolved
contracts/src/v0.8/ccip/capability/CCIPCapabilityConfiguration.sol Outdated Show resolved Hide resolved
contracts/src/v0.8/ccip/capability/CCIPCapabilityConfiguration.sol Outdated Show resolved Hide resolved
contracts/src/v0.8/ccip/capability/CCIPCapabilityConfiguration.sol Outdated Show resolved Hide resolved
contracts/src/v0.8/ccip/capability/CCIPCapabilityConfiguration.sol Outdated Show resolved Hide resolved
contracts/src/v0.8/ccip/capability/CCIPCapabilityConfiguration.sol Outdated Show resolved Hide resolved
contracts/src/v0.8/ccip/capability/CCIPCapabilityConfiguration.sol Show resolved Hide resolved
contracts/src/v0.8/ccip/capability/CCIPCapabilityConfiguration.sol
// staging -> running (promotion)
// everything else is invalid and should revert.
function _validateConfigStateTransition(ConfigState currentState, ConfigState newState) internal pure {
// TODO: may be able to save gas if we put this in the if condition.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: bit of an over-optimization but technically you just need to check newState - currentState, and allow it only if it is +1, or -1 if currentState = 2

Copy link
Contributor Author

@makramkd makramkd May 30, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that may not work due to the enums being unsigned integers (you won't get -1 but 255 or smth). I think we could just ditch enums and use signed int8's though. Will make a ticket for this.

contracts/src/v0.8/ccip/capability/CCIPCapabilityConfiguration.sol Show resolved Hide resolved
contracts/src/v0.8/ccip/capability/CCIPCapabilityConfiguration.sol Show resolved Hide resolved
@makramkd
CR feedback
0944c23 
* remove unused errors
* struct packing and comments
* some renames for clarity
* bytes32 instead of bytes in some places
* fixed size array instead of dynamic
* uint64[] array instead of ChainConfigUpdate
@makramkd
CR Changes
db6f3fc 
* gas optimization in getAllChainConfigs
* comment move to @/dev
cds95
cds95 reviewed May 30, 2024
View reviewed changes
contracts/src/v0.8/ccip/capability/CCIPCapabilityConfiguration.sol
/// @param configLen The length of the configuration.
/// @return The config state.
function _stateFromConfigLength(uint256 configLen) internal pure returns (ConfigState) {
if (configLen == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be useful to leave some @dev comments explaining how the configLens match to the ConfigState.

contracts/src/v0.8/ccip/capability/CCIPCapabilityConfiguration.sol Outdated Show resolved Hide resolved
contracts/src/v0.8/ccip/capability/CCIPCapabilityConfiguration.sol Outdated Show resolved Hide resolved
contracts/src/v0.8/ccip/capability/CCIPCapabilityConfiguration.sol Outdated Show resolved Hide resolved
contracts/src/v0.8/ccip/capability/CCIPCapabilityConfiguration.sol Outdated Show resolved Hide resolved
contracts/src/v0.8/ccip/capability/CCIPCapabilityConfiguration.sol Outdated Show resolved Hide resolved
contracts/src/v0.8/ccip/capability/CCIPCapabilityConfiguration.sol
/// @param chainSelectorRemoves The chain configurations to remove.
/// @param adds The chain configurations to add.
function applyChainConfigUpdates(
uint64[] calldata chainSelectorRemoves,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
uint64[] calldata chainSelectorRemoves,
uint64[] calldata removedChainSelectors,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They aren't removed yet when you pass them in the arg, might be a bit confusing naming it that way?

contracts/src/v0.8/ccip/capability/CCIPCapabilityConfiguration.sol
// Process removals first.
for (uint256 i = 0; i < chainSelectorRemoves.length; i++) {
// check if the chain selector is in s_chainSelectors first.
bool present = s_chainSelectors.contains(chainSelectorRemoves[i]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
bool present = s_chainSelectors.contains(chainSelectorRemoves[i]);
bool isValidChainSelector = s_chainSelectors.contains(chainSelectorRemoves[i]);

contracts/src/v0.8/ccip/capability/interfaces/ICapabilityConfiguration.sol
@@ -0,0 +1,36 @@
// SPDX-License-Identifier: MIT
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This interface has already been defined here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ya will have to import that instead when its available in ccip-develop

contracts/src/v0.8/ccip/capability/interfaces/ICapabilityRegistry.sol Show resolved Hide resolved
elatoskinas
elatoskinas approved these changes Jun 5, 2024
View reviewed changes
Copy link
Collaborator

@elatoskinas elatoskinas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elatoskinas elatoskinas elatoskinas approved these changes

@cds95 cds95 Awaiting requested review from cds95

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@makramkd @cds95 @elatoskinas