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

Feat/vault swaps ccm #242

Merged
merged 94 commits into from
Feb 20, 2023
Merged

Feat/vault swaps ccm #242

merged 94 commits into from
Feb 20, 2023

Conversation

albert-llimos
Copy link
Collaborator

@albert-llimos albert-llimos commented Jan 4, 2023

First draft of the updated logic and tests to enable cross chain messaging, both ingress and egress.
The intention of this PR is to enable both swapping through the Vault contract, also allowing CCM (cross-chain messaging).
Swapping through the Vault is enabled via xSwapNative and xSwapToken, which will basically emit an event that will be witnessed. xCallNative and xCallToken will allow for cross-chain calls with or without a swap at the same time (depending on the swapIntent). That call will be a call via cfReceive() on the egress chain with an additional message passed as a parameter. On the egress chain, that is done on via executexSwapAndCall. Also, in the case of a xCall, the destination native gas that the user wants to pay for, as well as a refundAddress needs to be specified. This is because the user will pay for the gas on the egress chain (we will take subtract it from the input amount and swap it to egress native token) and we will refund them with any remaining gas. The refunding part is not yet decided, but it would just be a matter of removing that parameter.

For all these functions I am particularly interested in discussing the interfaces. Please have in mind that those parameters are targeting all chains, not only EVM's. This is my reasoning for the parameters:

  • uint32 dstChain : We are targeting also non-EVM chains, so we can't use chainid. Therefore, it was either using a uint32 (and we have a list with the supported chains) or a string. I went for uint32 for gas efficiency.
  • string memory dstAddress: Not all chain addresses can be supported with bytes32. Bytes could be used but then chains that have strings as addresses will end up not being readable on chain while having the same overhead as strings. So strings seem more intuitive.
  • string memory swapIntent: This would signal the destination token or, in the case of xCall, an empty value would mean that all input tokens are used for gas. It could also be done as in dstChain (uint32), which would be more gas efficient. However, a string can make it more flexible for the future (e.g. being used to signal an action to take).
  • bytes calldata message: General bytes type.
  • uint256 dstNativeGas: Type used for gas.
  • address refundAddress: As of now the refund is thought for the ingress address, so that can be an address (as it's EVM).

Regarding the _executexSwapAndCall I have considered having a try-catch statement to ensure the tokens are transferred even in case of reverting of the call. However, I have decided against it for a few reasons: (just fyi, Axelar also does it this way)
1- When a swap is done via a DEX aggregator ( AGG -> CF -> CF -> AGG) they already have that safeguard to ensure the tokens end at the user's address. Having our try-catch that basically would end up transferring the tokens to the AGG contract doesn't solve anything.
2- If the dstAddress is an EOA, it's all good. If it's a contract, they have the flexibility to implement the try-catch or not, depending on what they want to do. We will clearly state that we advise that logic in case there is any risk of reverting.
3- In our protocol, we will have an easy time allowing for the user to replay the transaction in case of revertion. This is because anyone can send the signed transaction (msg.sender is irrelevant). Therefore, allowing it to revert makes it so users that prefer being able to replay a xCall instead of the tokens being transferred and the xCall being executed. It can happen that the xcall will never succeed when replayed, but in that case it is the user's responsibility to have implemented a try-catch or a way to ensure the xCall can succeed eventually.

Gas topups is commented out as it's unclear we want that in the contract at this point.

Topics for this PR:

  • Interfaces and of course any logic bug.
  • Opinions on views on all my design/logic choices explained above.
  • The main file to review is the Vault.sol. Also the CFReceiver, which is a template of what a receiver contract should have. The new added mocks (especially DexAggMock.sol and tests can be interesting to understand how it should work, but not really code we will deploy.
  • Don't review all the tests changes nor the changes to the Echidna contracts.

Still some open questions at the protocol level (not for this PR):

  • Do we want to future-proof it for retrospective refunds, passing that refund address?
  • Do we want to future-proof it with gas-topups functionality?
    Which enable signals do we need? swapsEnabled? xCallEnabled?

@albert-llimos albert-llimos removed the request for review from kylezs January 23, 2023 17:49
@albert-llimos albert-llimos marked this pull request as ready for review January 26, 2023 08:37
@albert-llimos
Copy link
Collaborator Author

albert-llimos commented Feb 15, 2023

Updates after some discussions:
string swapIntent uint16 dstToken: A uint signaling the desired destination token seems more straighforward than a string. Also cheaper gas-wise. We are giving up on the flexibility that strings provide, but we are fine with that.
uint256 dstNativeGas dstNativeBudget: Non-EVM chains don't necessarily have a concept of gas. Therefore, naming it gas can be confusing. Renaming it to a budget as that this will act as the gasLimit or potentially maximum price to pay for the gas.
string bytes dstAddress: Bytes will be more straightforward to interpret in the statechain, as different chains have different ways to represent addresses. Only downside is that it's less readable on-chain, but we are fine with that.

@albert-llimos
Copy link
Collaborator Author

albert-llimos commented Feb 16, 2023

Refund address to be changed from address to bytes, for consistency for how we expect addresses.. It is unclear if it will be used for gas refunds or for refunding failed swaps or any of that. A byte can also be used to represent ingress/egress addresses.

@albert-llimos albert-llimos merged commit fac51ac into master Feb 20, 2023
@albert-llimos albert-llimos deleted the feat/vault-swaps-ccm branch February 20, 2023 07:43
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.

None yet

3 participants