Conversation
| uint16 minBlockConfirmations | ||
| /// @notice Sets the finality config according to the FinalityCodec library encoding. | ||
| /// @param allowedFinality The finality settings allowed in this pool, according to the FinalityCodec encoding. | ||
| function setFinalityConfig( |
There was a problem hiding this comment.
To keep future expansion but also limit weird configurations we should have the tooling check for misconfigurations when setting this value. Things like setting WAIT_FOR_SAFE_FLAG to true on some chain that doesn't support that.
| _validateRequestedFinality(requestedFinality); | ||
|
|
||
| // If any of the flags match, the request is allowed only when it has no depth field (flag-only request). | ||
| if ((requestedFinality >> BLOCK_DEPTH_BITS) & (allowedFinality >> BLOCK_DEPTH_BITS) != 0) { |
There was a problem hiding this comment.
it's not immediately obvious if & or != 0 takes precedence, in quite some languages, != has higher precedence; let's add a paren here
There was a problem hiding this comment.
It's the bit math & not &&, are there any languages that bit math takes precedence over comparisons?
I'll add the parens though
| /// | ||
| /// @dev Bit layout of the `bytes2` finality value (16 bits, MSB on the left): | ||
| /// | ||
| /// Bit: 15 14 13 12 11 10 | 9 8 7 6 5 4 3 2 1 0 |
There was a problem hiding this comment.
random idea, do we wanna enshrine a bit, e.g leftmost bit, as a union flag, e.g set the flag to 1, then flags and depth become an "AND", as opposed to default "OR", for the potential special cases:
- people wanna introduce a minimum block delay, e.g. 100 blocks, but also ensure on the off-chance finality not reached by then, additionally wait for finality
- people not trusting FCR on some alt-L1 or L2s that wanna add additional depth protection
There was a problem hiding this comment.
We do not support any delay beyond finality as this would lead to unbounded delays in msg processing being potentially an attack vector.
If you do not trust a tag you must not use it.
| revert TokenHandlingError(localToken, err); | ||
| } | ||
| } else if (localPoolAddress._supportsInterfaceReverting(Pool.CCIP_POOL_V1)) { | ||
| try IPoolV1(localPoolAddress).releaseOrMint(releaseOrMintInput) returns (Pool.ReleaseOrMintOutV1 memory result) { |
There was a problem hiding this comment.
if we have pool asymmetry, e.g source pool is a v2 pool that uses FTF, dest pool is a v1 pool, seems this will succeed without finality validation; wanna sanity check that this is a known and explicitly accepted behavior
There was a problem hiding this comment.
Finality validation is done through _getCCVsFromPool, not here. In the case of a V1 pool it must use finality
| /// @return usdCentsFee The USD denominated fee for the executor. | ||
| function getFee( | ||
| uint64 destChainSelector, | ||
| uint16 blockConfirmationsRequested, |
There was a problem hiding this comment.
pool uses finalityConfig whereas verifier and executor uses requestedFinality
Finality Codec support in CLD
RensR
requested review from
0xAustinWang,
b-gopalswami,
chris-de-leon-cll,
mateusz-sekara,
ogtownsend,
tt-cll and
winder
# Conflicts: # ccv/chains/evm/deployment/v1_7_0/sequences/configure_chain_for_lanes_test.go # ccv/chains/evm/deployment/v1_7_0/sequences/tokens/configure_token_pool_for_remote_chain_test.go # deployment/tokens/configure_tokens_for_transfers.go # integration-tests/go.mod
|
We added an extra 2 bits to the finality config to allow for flag signals. This will support the upcoming FCR tag and potentially more in the future.