-
Notifications
You must be signed in to change notification settings - Fork 22
Lombard v2 pool backward compatibility #1440
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
69a6ca2 to
4d79408
Compare
|
|
||
| event LombardVerifierSet(address indexed verifier); | ||
| /// @param remoteChainSelector CCIP selector of destination chain. | ||
| /// @param lChainId The chain id of destination chain by Lombard Multi Chain Id conversion. |
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.
Alternatives:
- The Lombard-specific chain ID.
- The chain ID according to Lombard Multi Chain ID convention.
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.
fixed
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.
| /// @param remoteChainSelector CCIP selector of destination chain. | ||
| /// @param lChainId The chain id of destination chain by Lombard Multi Chain Id conversion. | ||
| /// @param allowedCaller The address of TokenPool on destination chain allowed to handle GMP message. | ||
| event PathSet(uint64 indexed remoteChainSelector, bytes32 indexed lChainId, bytes32 allowedCaller); |
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 PathSet & PathRemoved, we should specify if Lombard depends on them & how they are used given that they are named this way for a reason.
| uint8 fallbackDecimals | ||
| ) TokenPool(token, _getTokenDecimals(token, fallbackDecimals), advancedPoolHooks, rmnProxy, router) { | ||
| if (address(bridge) == address(0)) { | ||
| revert ZeroBridge(); |
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.
| revert ZeroBridge(); | |
| revert ZeroBridgeNotAllowed(); |
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.
WIll fix later after confirmation from Lombard, this is from the old code
|
|
||
| Path memory path = s_chainSelectorToPath[lockOrBurnIn.remoteChainSelector]; | ||
| if (path.allowedCaller == bytes32(0)) { | ||
| revert PathNotExist(lockOrBurnIn.remoteChainSelector); |
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.
| revert PathNotExist(lockOrBurnIn.remoteChainSelector); | |
| revert PathDoesNotExist(lockOrBurnIn.remoteChainSelector); |
| return super.lockOrBurn(lockOrBurnIn, blockConfirmationRequested, tokenArgs); | ||
| } | ||
|
|
||
| /// @notice Backwards compatible lockOrBurn for lanes using the V1 flow. |
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.
So the invariant here is that there will always be a pre 1.7.0 OffRamp available to process any in-flight or failed V1 Lombard messages? This is why we don't need any proxy?
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.
yes
| /// @param remoteChainSelector CCIP chain selector of remote chain. | ||
| /// @param lChainId Lombard chain id of remote chain. | ||
| /// @param allowedCaller The address of TokenPool on destination chain. | ||
| function setPath(uint64 remoteChainSelector, bytes32 lChainId, bytes calldata allowedCaller) external onlyOwner { |
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.
Better to just accept bytes32 for allowedCaller & abi.encode it for the isRemotePool check? Then we can remove the allowedCaller length check 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.
old Lombard function will discuss with them and change
| pragma solidity ^0.8.24; | ||
|
|
||
| /// @custom:security-contact legal@lombard.finance | ||
| interface IBridgeV2 { |
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.
There is a source control issue here where some files exist in both pools/Lombard and pools/lombard
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.
removed the pools/lombard folder
| address verifier, | ||
| IBridgeV2 bridge, | ||
| address adapter, | ||
| address advancedPoolHooks, |
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.
Let's remove the hooks from this pool including adding
/// @notice No-op override to purge the unused code path from the contract.
function _postFlightCheck(Pool.ReleaseOrMintInV1 calldata, uint256, uint16) internal pure virtual override {}
/// @notice No-op override to purge the unused code path from the contract.
function _preFlightCheck(Pool.LockOrBurnInV1 calldata, uint16, bytes memory) internal pure virtual override {}
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.
will do later, after discussion with Lombard
| /// @notice Mapping of CCIP chain selector to chain specific config. | ||
| mapping(uint64 chainSelector => Path path) internal s_chainSelectorToPath; | ||
|
|
||
| constructor( |
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.
Let's at least add comments for the lombard params, what an adapter is and that it's optional
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.
|
Backward compatibility based on
https://github.com/lombard-finance/evm-smart-contracts/blob/main/contracts/bridge/providers/LombardTokenPoolV2.sol
and
https://github.com/lombard-finance/evm-smart-contracts/blob/main/contracts/bridge/providers/BridgeTokenPool.sol