feat: on chain swaps #5595

msgmaxim · 2025-01-30T10:37:38Z

PRO-1998

Summary

Added extrinsic to the swapping pallet to initiate a swap with free balance (do we need to restrict it to account role == LP?)
Updated SwapRequestType (part of SwapRequests storage) where I replaced the output address with SwapOutputAction that has two variants (Egress - for "traditional" swaps, and CreditOnChain - for on chain swaps). Added migration for this.
Extended refund parameters so that the destination can also now be an on-chain account. The externally facing refund parameters struct remains unchanged (since we want to be able to reject/refund any ingress, and so internal account is not allowed as a refund destination).

Breaking changes

Changes to SwapRequested event:
- request_type in the Regular variant now can contain account_id instead of output_address.
- refund_params now contain refund_desitnation (instead of refund_address), which can be either an address (as before) or an account_id (in case of an internal swap).

Discussion

Do we need new events for when an account is credited as a result of a swap?
- Currently we are expecting the following sequence of events:

`SwapRequested` → `SwapScheduled` → `SwapExecuted` → `SwapEgressScheduled` (or `RefundEgressScheduled`) → `SwapRequestCompleted`

Presumably we want new events for internal swaps to match SwapEgressScheduled and RefundEgressScheduled.

msgmaxim · 2025-01-31T02:42:44Z

state-chain/pallets/cf-swapping/src/migrations/swap_and_swap_request_migration.rs

@@ -89,57 +89,6 @@ impl<T: Config> UncheckedOnRuntimeUpgrade for Migration<T> {
 	}

 	fn on_runtime_upgrade() -> Weight {
-		crate::SwapRequests::<T>::translate_values::<old::SwapRequest<T>, _>(|old_swap_requests| {


We need to branch 1.8 and remove this migration (unless we want this PR in 1.8, in which case I can fix this migration properly).

codecov · 2025-01-31T02:49:10Z

Codecov Report

Attention: Patch coverage is 83.74384% with 99 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...pping/src/migrations/on_chain_trading_migration.rs	0%	46 Missing ⚠️
state-chain/pallets/cf-lp/src/weights.rs	0%	18 Missing ⚠️
state-chain/pallets/cf-swapping/src/lib.rs	87%	14 Missing ⚠️
state-chain/traits/src/mocks/swap_request_api.rs	63%	4 Missing and 3 partials ⚠️
state-chain/chains/src/lib.rs	73%	4 Missing and 2 partials ⚠️
state-chain/traits/src/swapping.rs	77%	3 Missing and 2 partials ⚠️
state-chain/pallets/cf-pools/src/lib.rs	0%	0 Missing and 2 partials ⚠️
state-chain/pallets/cf-lp/src/lib.rs	97%	0 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

dandanlen

I think, for now at least, we probably do need to restrict this to LPs - LPs are likely to be the only parties with enough funds anyway.

state-chain/chains/src/lib.rs

state-chain/pallets/cf-swapping/src/lib.rs

dandanlen · 2025-01-31T15:59:11Z

state-chain/pallets/cf-swapping/src/lib.rs

+								);
+							},
+							SwapOutputAction::CreditOnChain { account_id } => {
+								// TODO: a new event for when LP is credited after a swap?


An event would make sense... we have an event for SwapExecuted, why not for SwapFailed?

I added CreditedOnChain and RefundedOnChain events.

We could also add SwapFailed though this isn't relevant to this PR and maybe should be discussed a little first? (e.g. do we emit one alongside every SwapRescheduled or only when finally refunding?)

Yes I agree, not part of this, PR, was just curious as to why we don't already have it - if we have an event for when a swap request succeeds, why not for when one fails? Did we just forget this or was there a reason?

Did we just forget this or was there a reason?

No reason IIRC other than needing some discussion. Because the frontend can use egress/refund events for this purpose, this hasn't been a priority. We should do it.

dandanlen

Now that the extrinsic is lp-specific, I would suggest refactoring such internal_swap is an interface method on SwapRequestHandler, and moving the extrinsic, account check, refund address check and sweep to the lp pallet.

Then the lp pallet can depend on the swapping interface rather than the swapping pallet depending on the lp and pools interfaces. This is more in line with how things are currently structured, with lp-specific calls and checks in the lp pallet. The swapping pallet has accumulated enough responsibilities already.

dandanlen

Btw. I noticed we already have a type AccountOrAddress in the lp pallet, it's very similar to RefundDestination, would be good to merge/deduplicate these.

dandanlen · 2025-02-14T14:46:05Z

state-chain/primitives/src/lib.rs

+/// zero becomes more valuable relative to asset one the price's literal value goes up, and vice
+/// versa. This ratio is represented as a fixed point number with `PRICE_FRACTIONAL_BITS` fractional
+/// bits.
+pub type Price = U256;


I've mentioned this many times - i'm not a fan of exposing a type alias like this as a 'primitive' when it's loaded with so much meaning...

I guess this is better than duplicating the definition though, so 🤷

Created PRO-2044 to address this once and for all.

dandanlen · 2025-02-14T14:55:12Z

state-chain/pallets/cf-lp/src/lib.rs

+					min_price,
+				}),
+				dca_params,
+				SwapOrigin::OnChainAccount(account_id),


nit: It might have been a bit cleaner to define a trait method like internal_swap_request(..), then all these types like SwapOrigin, SwapRequestType, SwapOutputAction and RefundDestination (even RefundParametersExtended?) could remain internal to the swapping pallet rather than leaking them in the public interface. And we would not have to pass empty broker fees (which makes little sense for an internal swap anyway).

feat: on chain swaps

a876c17

msgmaxim force-pushed the feat/on-chain-swaps branch from 8d4b190 to a876c17 Compare January 31, 2025 02:36

msgmaxim changed the title ~~WIP: on chain swaps~~ feat: on chain swaps Jan 31, 2025

msgmaxim commented Jan 31, 2025

View reviewed changes

dandanlen reviewed Jan 31, 2025

View reviewed changes

msgmaxim added 2 commits February 3, 2025 12:54

chore: address review

bd860b9

feat: add events for on-chain swapping

8154860

msgmaxim force-pushed the feat/on-chain-swaps branch from 482c012 to 8154860 Compare February 3, 2025 04:35

msgmaxim marked this pull request as ready for review February 3, 2025 04:37

msgmaxim added 2 commits February 3, 2025 15:41

chore: update openssl

8acf758

chore: fix benchmark

a8b0942

dandanlen reviewed Feb 3, 2025

View reviewed changes

msgmaxim added 3 commits February 4, 2025 12:03

Merge branch 'main' into feat/on-chain-swaps

f107b59

refactor: moving internal swaps to cf-lp

2cf2145

refactor: use AccountOrAddress; clippy fixes

2153e74

msgmaxim force-pushed the feat/on-chain-swaps branch from 4fffb16 to 2fab474 Compare February 12, 2025 01:50

Merge branch 'main' into feat/on-chain-swaps

d464d68

msgmaxim force-pushed the feat/on-chain-swaps branch from 2fab474 to d464d68 Compare February 12, 2025 01:50

dandanlen approved these changes Feb 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: on chain swaps #5595

feat: on chain swaps #5595

msgmaxim commented Jan 30, 2025 •

edited

Loading

msgmaxim Jan 31, 2025

codecov bot commented Jan 31, 2025 •

edited

Loading

dandanlen left a comment

dandanlen Jan 31, 2025

msgmaxim Feb 3, 2025

dandanlen Feb 3, 2025

msgmaxim Feb 4, 2025

dandanlen left a comment

dandanlen left a comment

dandanlen Feb 14, 2025

dandanlen Feb 14, 2025

feat: on chain swaps #5595

Are you sure you want to change the base?

feat: on chain swaps #5595

Conversation

msgmaxim commented Jan 30, 2025 • edited Loading

Summary

Breaking changes

Discussion

msgmaxim Jan 31, 2025

Choose a reason for hiding this comment

codecov bot commented Jan 31, 2025 • edited Loading

Codecov Report

dandanlen left a comment

Choose a reason for hiding this comment

dandanlen Jan 31, 2025

Choose a reason for hiding this comment

msgmaxim Feb 3, 2025

Choose a reason for hiding this comment

dandanlen Feb 3, 2025

Choose a reason for hiding this comment

msgmaxim Feb 4, 2025

Choose a reason for hiding this comment

dandanlen left a comment

Choose a reason for hiding this comment

dandanlen left a comment

Choose a reason for hiding this comment

dandanlen Feb 14, 2025

Choose a reason for hiding this comment

dandanlen Feb 14, 2025

Choose a reason for hiding this comment

msgmaxim commented Jan 30, 2025 •

edited

Loading

codecov bot commented Jan 31, 2025 •

edited

Loading