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: Boost Support for Vault Swaps #5437

Merged
merged 14 commits into from
Dec 4, 2024
Merged

Feat: Boost Support for Vault Swaps #5437

merged 14 commits into from
Dec 4, 2024

Conversation

msgmaxim
Copy link
Contributor

@msgmaxim msgmaxim commented Nov 21, 2024

Pull Request

Closes: PRO-1753

Summary

  • Added additional fields to vault_swap_request (height, deposit_address, channel_id). This is needed for DepositBoosted/DepositFinalised events. I also made it take a vec of deposits (represented as VaultDepositWitness) instead to be consistent with "regular" deposits and as a better (compared to box-ing individual fields) solution to the "max call size" problem that occurs when we have too many paramters not on a heap. Right now engines still vote on individual deposits (i.e. vec of size 1), but we can change this in the future and submit batches of deposits like we do for regular deposits (@dandanlen mentions that this might require some discussion, so not done here).

  • Added BoostedVaultTxs field to track the status of vault deposits (regular deposits use DepositChannelLookup for this)

  • Added DepositOrigin which mimicks SwapOrigin but excludes Internal variant and makes more sense in contexts where we are not necessarily dealing with swaps, but deposits in general. I considered merging them into one to simplify, but seemed a bit cleaner to have explicit types (but happy to reconsider).

  • Factored out process_prewitness_deposit_inner and process_full_witness_deposit_inner so that the core logic is used regardless of the "origin". Deposit extrinsics now just assemble the parameters for the channel action, perform validation where needed, and read/update the boost status.

  • DepositBoosted/DepositFinalised events now include DepositOriginType so the product knows whether channel_id should be used to lookup deposit channel details for example. @niklasnatter

Edit: I might need to check unit-tests/bouncer tests to see why they fail, but otherwise this PR should be read for review.

dca_params,
swap_origin,
);
let (action, source_address) = if let Some(deposit_metadata) = deposit_metadata {
Copy link
Contributor Author

@msgmaxim msgmaxim Nov 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a cleaner solution for extracting/passing source_address might be to create a different struct e.g. DepositAction which will include CcmDepositMetadata instead of CcmChannelMetadata in ChannelAction (and/or making ChannelAction generic over metadata, though this would make the code less readable), but I chose to go with a solution that requires fewer code changes for now.

@msgmaxim msgmaxim force-pushed the feat/vault-swap-boost branch from 69a23d6 to 92d1953 Compare November 22, 2024 02:03
Copy link

codecov bot commented Nov 22, 2024

Codecov Report

Attention: Patch coverage is 87.68529% with 108 lines in your changes missing coverage. Please review.

Project coverage is 72%. Comparing base (d12200a) to head (573982a).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
state-chain/pallets/cf-ingress-egress/src/lib.rs 93% 29 Missing and 2 partials ⚠️
...te-chain/runtime/src/chainflip/solana_elections.rs 0% 19 Missing ⚠️
engine/src/witness/arb.rs 0% 13 Missing ⚠️
engine/src/witness/btc/deposits.rs 0% 13 Missing ⚠️
engine/src/witness/eth.rs 0% 12 Missing ⚠️
engine/src/witness/evm/vault.rs 0% 7 Missing ⚠️
state-chain/traits/src/mocks/balance_api.rs 25% 6 Missing ⚠️
state-chain/pallets/cf-lp/src/mock.rs 67% 3 Missing ⚠️
...ate-chain/pallets/cf-ingress-egress/src/weights.rs 0% 2 Missing ⚠️
state-chain/chains/src/lib.rs 80% 0 Missing and 1 partial ⚠️
... and 1 more
Additional details and impacted files
@@          Coverage Diff           @@
##            main   #5437    +/-   ##
======================================
  Coverage     72%     72%            
======================================
  Files        494     494            
  Lines      87751   87926   +175     
  Branches   87751   87926   +175     
======================================
+ Hits       62826   62982   +156     
- Misses     22310   22365    +55     
+ Partials    2615    2579    -36     

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

@albert-llimos
Copy link
Contributor

albert-llimos commented Nov 25, 2024

channel_id is not needed and actually makes no sense outside of Bitcoin, as the premise of Vault swaps is to not need a channel. Therefore passing a channel_id as a mandatory parameter to vault_swap_request and arbitrarily hardcoding that to zero for other chains seems misleading to me. It could even be just wrong for chains were channels are reused as we could currently be using that channel.
I'd suggest making that an Option. EDIT: Or if we can fit it into DepositDetails or some other parameter that could make sense.

Copy link
Contributor

@kylezs kylezs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pretty minor comments overall 👌

engine/src/witness/btc/deposits.rs Outdated Show resolved Hide resolved
state-chain/pallets/cf-ingress-egress/src/lib.rs Outdated Show resolved Hide resolved
state-chain/pallets/cf-ingress-egress/src/lib.rs Outdated Show resolved Hide resolved
state-chain/pallets/cf-ingress-egress/src/lib.rs Outdated Show resolved Hide resolved
state-chain/pallets/cf-ingress-egress/src/lib.rs Outdated Show resolved Hide resolved
state-chain/pallets/cf-ingress-egress/src/lib.rs Outdated Show resolved Hide resolved
state-chain/pallets/cf-ingress-egress/src/lib.rs Outdated Show resolved Hide resolved
state-chain/pallets/cf-ingress-egress/src/lib.rs Outdated Show resolved Hide resolved
@msgmaxim msgmaxim force-pushed the feat/vault-swap-boost branch 2 times, most recently from 4f3741e to 79a92b9 Compare December 2, 2024 23:55
@msgmaxim msgmaxim force-pushed the feat/vault-swap-boost branch from 79a92b9 to acaebcf Compare December 3, 2024 02:02
state-chain/chains/src/lib.rs Show resolved Hide resolved
state-chain/pallets/cf-ingress-egress/src/lib.rs Outdated Show resolved Hide resolved
@msgmaxim msgmaxim enabled auto-merge December 4, 2024 06:08
@msgmaxim msgmaxim added this pull request to the merge queue Dec 4, 2024
Merged via the queue into main with commit 74e50be Dec 4, 2024
49 checks passed
@msgmaxim msgmaxim deleted the feat/vault-swap-boost branch December 4, 2024 07:33
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.

5 participants