Feat: EVM Vault swaps support for Broker and affiliates #5408
Conversation
…inflip-io/chainflip-backend into feat/cf-parameters-broker-affiliates
albert-llimos
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5408 +/- ##
======================================
- Coverage 72% 72% -0%
======================================
Files 489 489
Lines 86812 86691 -121
Branches 86812 86691 -121
======================================
- Hits 62339 62187 -152
- Misses 21555 21578 +23
- Partials 2918 2926 +8 ☔ View full report in Codecov by Sentry. |
albert-llimos
requested review from
kylezs and
martin-chainflip
as code owners
| @@ -22,18 +26,26 @@ pub struct VaultSwapParameters { | |||
| pub refund_params: ChannelRefundParameters, | |||
| pub dca_params: Option<DcaParameters>, | |||
| pub boost_fee: Option<BasisPoints>, | |||
There was a problem hiding this comment.
Unrelated to this PR but just noticed this is Option<BasisPoints> - should it not just be BasisPoints? We can save a byte 😅
There was a problem hiding this comment.
Originally when I wrote this my thought was that 1 byte makes no difference in any chain (not BTC as we don't use that there) and that it actually makes more sense to use None instead of boost of zero. However seeing that the extrinsic doesn't take an option it makes sense to remove that for consistency.
I'll change that, I can push it on top of this now that I'm also modifying the SDK.
There was a problem hiding this comment.
Technically the option saves one byte compared to using BasisPoints when boost is not being used, which is actually for all swaps as we don't support boost for any of these chains (EVM/SOL).
Makes sense to change it anyway for consistency as 1 byte makes no difference as per my previous comment.
There was a problem hiding this comment.
Or you could just use a single byte (u8) like we did for BTC 🙂
engine/src/witness/arb.rs
Outdated
| .expect("runtime supports at least as many affiliates as we allow in cf_parameters encoding"), | ||
| boost_fee: vault_swap_parameters.boost_fee.unwrap_or_default(), | ||
| dca_params: vault_swap_parameters.dca_params, | ||
| refund_params: Some(Box::new(vault_swap_parameters.refund_params)), |
There was a problem hiding this comment.
How come refund params are optional on the extrinsic but we never use None?
There was a problem hiding this comment.
I guess it's because we wanted to have some flexibility as we've been going back and forth between making this mandatory or not. I think we could consider removing the option since we are now enforcing it in Vault swaps. Wdyt? @dandanlen @msgmaxim
There was a problem hiding this comment.
Yeah, let's remove Option
|
|
||
| const MAX_VAULT_SWAP_PARAMETERS_LENGTH: u32 = 1_000; | ||
| const MAX_CF_PARAM_LENGTH: u32 = | ||
| MAX_CCM_ADDITIONAL_DATA_LENGTH + MAX_VAULT_SWAP_PARAMETERS_LENGTH; | ||
|
|
||
| const REFERENCE_EXPECTED_ENCODED: &[u8] = &[ |
There was a problem hiding this comment.
Maybe subjective, but I kind of like the way I broke down individual fields in check_utxo_encoding (https://github.com/chainflip-io/chainflip-backend/blob/main/state-chain/chains/src/btc/vault_swap_encoding.rs#L144) so we could see which bytes correspond to which field. Probably more important for BTC though where we were trying to compress the encoding.
|
|
||
| // Extra byte for the empty ccm metadata | ||
| let mut expected_encoded = vec![0]; | ||
| expected_encoded.extend_from_slice(REFERENCE_EXPECTED_ENCODED); |
There was a problem hiding this comment.
Nit: I believe you can just convert to Vec with Vec::from (or alternatively compare slices with &encoded[..] or something like that)
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Checklist
Please conduct a thorough self-review before opening the PR.
Summary
Add affiliates and broker account support for EVM Vault Swaps as a followup of #5389 .
Updates in SDK:
chainflip-io/chainflip-sdk-monorepo@e928861
chainflip-io/chainflip-sdk-monorepo@cbf09e2