feat: add/update contract swaps parameters

[msgmaxim]

Pull Request

Closes: PRO-1731, PRO-1732

• Added missing parameters (FoK, DCA, Boost) to contract swap extrinsics (contract_swap_request and contract_ccm_swap_request); these parameters are now passed into the swapping pallet and will be used if provided. (fyi @albert-llimos)

• Made some parameters more consistent (and to make sure we don't have to assert/convert when passing them further):

  • from type changed Asset -> TargetChainAsset<T, I>
  • deposit_amount type changed AssetAmount -> <T::TargetChain as Chain>::ChainAmount

• Contract swaps now validate DCA/FoK parameters and enforce minimum deposit amount

• Most code in both extrinsics is the same, so I factored it out into process_contract_swap_request

BREAKING CHANGES (fyi @acdibble)

• DepositIgnored event: deposit_address is now optional; this to be able to ignore deposits made directly into our valut.
• CcmFailed was emitted on invalid destination address in case ccm parameters were provided. I made it more consistent with non-ccm contract swaps where we just log a warning, which also made the code simpler. (CcmFailed is still emitted when a deposit/swap fails for ccm-sepcific reason.) As such, I also removed the CcmFailReason::InvalidDestinationAddress variant.

Discussion:

• Can we not just make (another) breaking change and replace both extrinsics with one (simply contract_swap_request) that supports both CCM and non-CCM by acception an optional ccm_deposit_metadata parameter?
• CcmFailed seems odd. We don't really have any other events for when other parameters are invalid. Is it used anywhere? Can we simplify code by removing it and replacing it with a warning (like we do for other parameters).

[albert-llimos]

Can we not just make (another) breaking change and replace both extrinsics with one (simply contract_swap_request) that supports both CCM and non-CCM by acception an optional ccm_deposit_metadata parameter?

That makes sense to me. Especially when we implement the changes in CCM swaps, regular swaps and CCM swaps will be almost identical for the majority of the swap flow. So I'd be in favor of doing it if the breaking change is not annoying to handle on upgrade.

[dandanlen]

Yeah let's just merge these. They can only be called via witnessing so for backwards-compatibility / to avoid migration headaches, we could keep and deprecate the old ones and add a new one. Then we can remove the new one at a later release.

[msgmaxim]

Haven't we already broken compatibility by adding new parameters?

[dandanlen]

As discussed - I guess we did 😅

But given that we are now aware of this, let's avoid breaking - it's easy to do.

[dandanlen]

CcmFailed seems odd.

This is/was to communicate (visibly) that something went wrong, without returning an error, because returning an error in the extrinsic rolls back the state, and we don't want to do that in a witnessed transaction.

If anything, we should emit a failure event for the address decoding too.

A cleaner approach might be to create an Error enum and return that from process_contract_swap_request (then we can write the code a bit more concisely/idiomatically), and log that error in an event, say SwapRequestFailed { swap_origin: SwapOrigin, reason: SwapRequestError, .. }.

[albert-llimos]

As discussed in the ticket below, and more particularly on the comment, we'll need to pass the accounts and commission/fees for both the broker and the affiliates. Do we use this parameter for both (e.g. first account is the broker) or we separate them?
This also runs into what I recently discussed with @dandanlen on whether we should make passing the brokerId mandatory for Vault swaps.
https://linear.app/chainflip/issue/PRO-1743/support-for-brokeraffiliate-fees-via-vault-swaps
https://linear.app/chainflip/issue/PRO-1743/support-for-brokeraffiliate-fees-via-vault-swaps#comment-338490a3
I'm not sure there is an easy answer for whether it should be mandatory but it can make sense to separate them here just for flexibility in the future. That's either by having two separate parameters or by this broker_fees having two separate parameters.

[dandanlen]

I think this approach is fine for now - we don't need to worry too much about breaking changes here because it only really affects us (it's a witnessed call), and isn't something that is exposed externally.

[codecov]

Codecov Report

Attention: Patch coverage is 77.33333% with 85 lines in your changes missing coverage. Please review.

Project coverage is 71%. Comparing base (dc8bf3e) to head (2ff3128).
Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
state-chain/pallets/cf-ingress-egress/src/lib.rs	74%	24 Missing and 7 partials ⚠️
engine/src/witness/eth.rs	0%	26 Missing ⚠️
engine/src/witness/arb.rs	0%	22 Missing ⚠️
state-chain/runtime/src/lib.rs	0%	0 Missing and 4 partials ⚠️
...ate-chain/traits/src/mocks/swap_limits_provider.rs	82%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@          Coverage Diff           @@
##            main   #5343    +/-   ##
======================================
- Coverage     71%     71%    -0%     
======================================
  Files        495     495            
  Lines      86335   86286    -49     
  Branches   86335   86286    -49     
======================================
- Hits       61623   61509   -114     
- Misses     21951   22009    +58     
- Partials    2761    2768     +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[dandanlen]

LGTM 👌

[dandanlen]

Sorry that commit should be Merge branch 'main'.... not sure what happened there.