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.

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

RMN Home #1451

Closed
wants to merge 13 commits into from
Closed

RMN Home #1451

wants to merge 13 commits into from

Conversation

RensR
Copy link
Collaborator

@RensR RensR commented Sep 19, 2024

Still WIP, but overal structure should be pretty solid

uint64 minObservers; // required to agree on an observation for this source chain
uint64 chainSelector; // ─────╮ The Source chain selector.
uint64 minObservers; // ──────╯ Required number of observers to agree on an observation for this source chain.
uint256 observerNodesBitmap; // ObserverNodesBitmap & (1<<i) == (1<<i) iff nodes[i] is an observer for this source chain.
Copy link
Contributor

Choose a reason for hiding this comment

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

@gtklocker does it mean that we can have a max of 256 observers per src chain?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, that should be plenty right?

Copy link
Contributor

github-actions bot commented Sep 19, 2024

LCOV of commit 32a3cda during Solidity Foundry #8309

Summary coverage rate:
  lines......: 97.0% (2349 of 2422 lines)
  functions..: 93.7% (433 of 462 functions)
  branches...: 92.3% (552 of 598 branches)

Files changed coverage rate: n/a

⛔ The code coverage is too low: 96.99. Expected at least 97.6.

}
/// @notice This array holds the digests of the configs, used for efficiency.
/// @dev Value i in this array is valid iff it's not 0.
bytes32[MAX_CONCURRENT_CONFIGS] private s_configDigests;
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we really care about efficiency here? CCIPConfig has just the one mapping with OCR3ConfigWithMeta holding configCount (same as this version) and digest

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll modify the comment. The real reason for this layout is that Solidity doesn't let you assign complex memory structs to storage. It does allow you to copy them from calldata, as that's not memory. This is how we can store the config to storage without manually copy-pasting each attribute (very error prone, not readable, inefficient) like we do in CCIPConfig. Imo RMN uses the superior solution here. We would have used a single list if this wasn't the case

Config config;
}

struct VersionedConfigWithDigest {
Copy link
Collaborator

Choose a reason for hiding this comment

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

is there a way to combine VersionedConfigWithDigest + VersionedConfig into RMNConfigWithMeta (just so analogous with OCR3ConfigWithMeta)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I just tried, the struct is too complex and will always result in a stack too deep if we use the same format as OCR3ConfigWithMeta

}

struct VersionedConfig {
uint32 version;
uint32 version; // The version of this config, starting from 1 it increments with each new config.
Copy link
Collaborator

Choose a reason for hiding this comment

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

could we align with OCR3ConfigWithMeta in the sense that the version is just called configCount? Fine if we adjust CCIPConfig to use version instead of configCount as well

}
}
/// @notice The total number of configs ever set, used for generating the version of the configs.
uint32 private s_configCount = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

CCIPConfig avoids this by incrementing based on the existing config (if no config, start at 1). Could probably do the same here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It can only do so by not supporting green-green, which imo is a huge feature loss

/// @param newConfig The new config to set.
/// @param digestToOverwrite The digest of the config to overwrite, or ZERO_DIGEST if no config is to be overwritten.
/// This is done to prevent accidental overwrites.
function setSecondary(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I can't see a fundamental reason why the same pattern (either explicit functions per state transition like you have it now in RMNHome OR single updateConfig with validateStateTransition for all transitions in CCIPConfig) can't be used in both contracts so we should pick one to reduce the cognitive load

(_getConfigDigestPrefix() & PREFIX_MASK)
| (
uint256(
keccak256(bytes.concat(abi.encode(bytes32("EVM"), block.chainid, address(this), version), staticConfig))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@gtklocker I did bytes.concat to not abi.encode the static config twice, that should be completely safe as the previous items have a static length? Alternatively we could hash it and then include the hash in the abi.encode?

_validateDynamicConfigParsed(abi.decode(encodedDynamicConfig, (DynamicConfig)), staticConfig.nodes.length);
}

function _validateDynamicConfigParsed(DynamicConfig memory dynamicConfig, uint256 numberOfNodes) internal pure {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added this helper to not have to parse the static config twice (once for static checks, once for dynamic checks)

@cl-sonarqube-production
Copy link

Quality Gate passed Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarQube

@RensR
Copy link
Collaborator Author

RensR commented Sep 23, 2024

closed in favour of #1459

@RensR RensR closed this Sep 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants