Make router destination/source chain specific #1242
Conversation
RyanRHall
requested review from
makramkd,
elatoskinas,
RayXpub and
a team
as code owners
app-token-issuer-infra-releng
bot
requested a review
from a team
as a code owner
LCOV of commit
|
RyanRHall
force-pushed
the
CCIP-2837-multi-on-ramp-make-router-destination-chain-specific-2
branch
from
041611f to
46f5117
Compare
RyanRHall
force-pushed
the
CCIP-2837-multi-on-ramp-make-router-destination-chain-specific-2
branch
from
267a5c8 to
d4bb04d
Compare
| /// @notice Gets the source router for a destination chain | ||
| /// @param destChainSelector The destination chain selector | ||
| /// @return destChainConfig the config for the provided destination chain | ||
| function getDestChainConfig(uint64 destChainSelector) external view returns (DestChainConfig memory) { |
There was a problem hiding this comment.
suggestion: there is already a function to fetch the sequence number. I think we should do 1 of the 2 options:
- Remove
getExpectedNextSequenceNumberfunction - Replace this function to only return the router
Personally I'm in favour of 1, I think there is also a benefit off-chain to know which router it is in the same flow. This would require some off-chain rework though
There was a problem hiding this comment.
I have some follow up Qs here - personally I'm usually in favor of "individual getters, monolithic setters" - ex setDestChainConfig(..), getRouter(destChain) and getSequenceNum(destChain). Especially if there is an offchain component here that might need to know about the router or seq num. The reason is for ease in upgrading. If we return all those fields in a single getConfig() function and then add something to the config, it's going to break the read function for offchain components, and those components will have to become version aware. Basically, I think individial getters make it easier for us to make patch or minor version upgrades.
There was a problem hiding this comment.
Do you guys have an opinion on getExpectedNextSequenceNumber() vs getSequenceNumber()???
There was a problem hiding this comment.
Personally in favour of getSequenceNumber() as the increment can always be done off-chain (and on-chain we do not use this function). However, this might break some code in chainlink-ccip
There was a problem hiding this comment.
I'm also pro getSequenceNumber() - I'll save this for a follow up and we'll see if it's easy to coordinate with the other repo.
There was a problem hiding this comment.
I also prefer getSequenceNumber() but we should confirm with off-chain first.
RyanRHall
force-pushed
the
CCIP-2837-multi-on-ramp-make-router-destination-chain-specific-2
branch
from
a46e289 to
1eeb738
Compare
…destination-chain-specific-2
| @@ -58,12 +61,29 @@ contract EVM2EVMMultiOnRamp is IEVM2AnyOnRampClient, ITypeAndVersion, OwnerIsCre | |||
| /// @dev Struct to contains the dynamic configuration | |||
| // solhint-disable-next-line gas-struct-packing | |||
| struct DynamicConfig { | |||
| address router; // Router address | |||
| address priceRegistry; // Price registry address | |||
There was a problem hiding this comment.
suggestion: since we're using specific types (IRouter) - should we create a new ticket to convert these to specific types across all contracts? E.g. for dynamic config:
struct DynamicConfig {
IPriceRegistry priceRegistry;
IMessageValidator messageValidator;
address feeAggregator;
}There was a problem hiding this comment.
👍 I'll create the ticket
| assertEq(address(s_sourceRouter), address(s_onRamp.getRouter(DEST_CHAIN_SELECTOR))); | ||
| assertEq(address(9999), address(s_onRamp.getRouter(9999))); | ||
|
|
||
| // handles empty list |
There was a problem hiding this comment.
nit: would be cleaner to split this into a separate test case
…destination-chain-specific-2
…destination-chain-specific-2
| continue | ||
| } | ||
| onrampSourceChainConfigArgs = append(onrampSourceChainConfigArgs, evm_2_evm_multi_onramp.EVM2EVMMultiOnRampDestChainConfigArgs{ | ||
| DestChainSelector: getSelector(remoteChainID), // for each destination chain, add a source chain config |
There was a problem hiding this comment.
Might be some copy-pasta here but this comment seems misleading, either remove or replace it with something more useful, e.g:
// Setup a router for each destination chainThere was a problem hiding this comment.
yes, poor technique in my copy-pasta. good catch!
| @@ -704,6 +723,7 @@ func wireOffRamp(t *testing.T, uni onchainUniverse, universes map[uint64]onchain | |||
| offrampSourceChainConfigArgs = append(offrampSourceChainConfigArgs, evm_2_evm_multi_offramp.EVM2EVMMultiOffRampSourceChainConfigArgs{ | |||
| SourceChainSelector: getSelector(remoteChainID), // for each destination chain, add a source chain config | |||
There was a problem hiding this comment.
Also misleading comment, maybe:
| SourceChainSelector: getSelector(remoteChainID), // for each destination chain, add a source chain config | |
| SourceChainSelector: getSelector(remoteChainID), // for each source chain, add a source chain config |
Though seems kinda redundant so I'd say just remove it
…destination-chain-specific-2
…destination-chain-specific-2
RyanRHall
deleted the
CCIP-2837-multi-on-ramp-make-router-destination-chain-specific-2
branch
|
## Motivation We want the ability to test new lanes using custom, test routers. Therefore, we need a one-to-many relationship b/t routers and lanes, rather than a singleton. ## Solution Make routers configurable per lane, rather than per contract
Motivation
We want the ability to test new lanes using custom, test routers. Therefore, we need a one-to-many relationship b/t routers and lanes, rather than a singleton.
Solution
Make routers configurable per lane, rather than per contract