-
Notifications
You must be signed in to change notification settings - Fork 22
[CCIP-7559] Add CCTPVerifier Contract #1396
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
base: develop
Are you sure you want to change the base?
Conversation
| event FinalityConfigSet(FinalityConfig finalityConfig); | ||
|
|
||
| /// @notice The static configuration. | ||
| // solhint-disable-next-line gas-struct-packing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you ignore this for every struct? They appear trivial to pack
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because these aren't ever stored, they are just used in memory. The comments note that we don't care about struct packing for those specific structs. I had seen it used in a similar way on other contracts. I can remove it though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
|
||
| /// @notice Configures finality handling for this chain. | ||
| struct FinalityConfig { | ||
| uint32 defaultCCTPFinalityThreshold; // ──╮ CCTP finality threshold applied when CCIP finality is 0. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this not be a constant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Responded to most finality threshold comments elsewhere. If referring to the default (2000), then yea we could make it a constant because I doubt CCTP would ever change that (though this is never stated explicitly).
| // The maximum fee, taken on destination, is a percentage of the total amount transferred. | ||
| // We use bps to calculate the smallest possible value that we can set as the max fee. | ||
| // The bps values configured for each finality threshold on this chain must mirror those used by CCTP. | ||
| // CCTP defines different bps values for each chain. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not exist as something we can check on their contracts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this is added to the CCTP message by Iris. Field is called feeExecuted.
| ) private view returns (uint32 finalityThreshold, uint16 bps) { | ||
| if (finality != 0) { | ||
| // The length of the customCCIPFinalities array is guaranteed to be at least 1. | ||
| // Therefore, if finality is not 0, we will find a match here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CCTP has two modes, right? This feels overly complex. Should we not simply use 2 hard coded values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now yes it has two modes. We can hard-code, just means we have to change this contract if they ever add new thresholds that we want to support. CCTP V2 white paper notes this as a possibility.
| /// @notice Sets the finality configuration for this chain. | ||
| /// @dev This function validates that custom finality values are sorted in ascending order. | ||
| /// @param finalityConfig The finality configuration. | ||
| function _setFinalityConfig( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If possible we kill all this and use
if x == 0
return 1000
else
return 2000
Those are the only allowed speeds right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can hard-code, just means we have to change this contract if they ever add new thresholds that we want to support. CCTP V2 white paper notes this as a possibility.
|
https://smartcontract-it.atlassian.net/browse/CCIP-7559
Out of scope: