Feat: Scheduled Swaps Subscription #4525

msgmaxim · 2024-02-14T07:52:38Z

Pull Request

Checklist

Please conduct a thorough self-review before opening the PR.

I am confident that the code works.
I have updated documentation where appropriate.

Summary

Took me a bit longer than it should have... I initially thought we should need a stream of finalized stream, and I wrote some code to deal with that, only to realise that with a finalized stream a 2 block swap delay wouldn't be long enough. So here I'm exposing swaps from unfinalised blocks using existing new_subscription.

I also ran into an issue after merging recent changes in main. Took me some time to realise this, but depending on what we use as T in new_subscription, we could get a silent failure when pipe_from_try_stream was failing to encode BlockUpdate internally (due to serde(flatten)), and this wasn't closing the subscription either. I now added error handling around pipe_from_try_stream to make it easier detect failures like this in the future.

Example usage:

wscat -c ws://localhost:9944
> {"id":1, "jsonrpc":"2.0", "method": "cf_subscribe_scheduled_swaps", "params": ["FLIP", "BTC"]}

This would return a notification on every block, even when the list of swaps is empty:

{"jsonrpc":"2.0","method":"cf_subscribe_scheduled_swaps","params":{"subscription":"SgUMJq00PQ7tetsx","result":{"block_hash":"0xdb82a90439be5826f8dece231d35ee8b23c9baaf98839aa345fc188bdee64423","block_number":58,"swaps":[{"swap_id":1,"from":"BTC","to":"FLIP","amount":4949884,"swap_type":{"Swap":{"Eth":"0x41ad2bc63a2059f9b623533d87fe99887d794847"}},"stable_amount":null,"final_output":null,"fee_taken":false,"execute_at":60}]}}}
< {"jsonrpc":"2.0","method":"cf_subscribe_scheduled_swaps","params":{"subscription":"SgUMJq00PQ7tetsx","result":{"block_hash":"0x4f70862ca1544f5add9c61598a0e1875e9ca3dd9637e243f0cfb68cefdb7db3a","block_number":59,"swaps":[{"swap_id":1,"from":"BTC","to":"FLIP","amount":4949884,"swap_type":{"Swap":{"Eth":"0x41ad2bc63a2059f9b623533d87fe99887d794847"}},"stable_amount":null,"final_output":null,"fee_taken":false,"execute_at":60}]}}}
< {"jsonrpc":"2.0","method":"cf_subscribe_scheduled_swaps","params":{"subscription":"SgUMJq00PQ7tetsx","result":{"block_hash":"0xc5737b08cf7133c1fdb88f7932f1b8c67945e216c9e5101988323ad1e627f2af","block_number":60,"swaps":[]}}}

Note how the same swap is returned twice (i.e. in two notifications) in this case. This is becasue we return all swaps that are "pending", and it takes 2 blocks untils swaps get executed.

codecov · 2024-02-14T08:06:56Z

Codecov Report

Attention: Patch coverage is 64.20455% with 63 lines in your changes are missing coverage. Please review.

Project coverage is 72%. Comparing base (307485c) to head (a2ef796).

Files	Patch %	Lines
state-chain/custom-rpc/src/lib.rs	0%	45 Missing ⚠️
state-chain/runtime/src/lib.rs	0%	0 Missing and 14 partials ⚠️
state-chain/pallets/cf-swapping/src/lib.rs	94%	1 Missing and 2 partials ⚠️
state-chain/runtime/src/runtime_apis.rs	67%	1 Missing ⚠️

Additional details and impacted files

@@          Coverage Diff           @@
##            main   #4525    +/-   ##
======================================
- Coverage     73%     72%    -0%     
======================================
  Files        401     401            
  Lines      66331   66367    +36     
  Branches   66331   66367    +36     
======================================
- Hits       48193   48101    -92     
- Misses     15814   15929   +115     
- Partials    2324    2337    +13

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

state-chain/runtime/src/lib.rs

state-chain/custom-rpc/src/lib.rs

state-chain/runtime/src/lib.rs

state-chain/custom-rpc/src/lib.rs

kylezs · 2024-02-14T09:44:47Z

Once this is included we should add docs to the docs repo, I added a sub-issue on linear so we don't forget: PRO-1203

kylezs · 2024-02-14T10:04:45Z

Rest looks fine to me. Will let @AlastairHolmes approve as he's been working on these RPCs a lot recently

AlastairHolmes · 2024-02-14T12:24:50Z

state-chain/runtime/src/lib.rs

@@ -1408,6 +1408,25 @@ impl_runtime_apis! {
 			all_prewitnessed_swaps
 		}

+		fn cf_scheduled_swaps(from: Asset, to: Asset) -> Vec<(Swap, BlockNumber)> {


These should be consistent with the other interfaces, and therefore called base_asset and quote_asset.

And the ordering should be enforced to match the pool.

Right, I see a problem. I believe what we want from this sub is per pool scheduled swaps, otherwise the sub isn't very useful. Where as my (somewhat brief) reading of this, is this implementation is a sub of end to end scheduled swaps. The problem with that is LPs would have to do not insignificant amount of work to work out what the per pool scheduled swaps are.

AlastairHolmes · 2024-02-14T12:48:28Z

Ignore my comments, I've realised there's been some miss-communication about the intended behaviour. I will write on the ticket (Shortly, not immediately (Lunch)).

kylezs

Looks good to me, will let @AlastairHolmes hit approve

AlastairHolmes · 2024-02-20T12:55:06Z

state-chain/custom-rpc/src/lib.rs

+	// Subscribe to a stream that on every block produces a list of all scheduled/pending
+	// swaps in the base_asset/quote_asset pool, including any "implicit" half-swaps (as a
+	// part of a swap involving two pools)
+	#[subscription(name = "subscribe_scheduled_swaps", item = SwapResponse)]


This should be BlockUpdate<SwapResponse> right?

AlastairHolmes · 2024-02-20T13:00:24Z

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

+#[cfg_attr(feature = "std", derive(serde::Serialize, serde::Deserialize))]
+pub struct SwapLegInfo {
+	pub swap_id: SwapId,
+	pub from: Asset,


Instead of from and to, we should use base_asset, quote_asset, and side: Order (This type is being changed to be called Side fyi, by Jan).

I'd like to do this for consistency with the new rpcs.

AlastairHolmes · 2024-02-20T14:27:04Z

state-chain/custom-rpc/src/lib.rs

+			sink,
+			move |api, hash| {
+				let swaps = api
+					.cf_scheduled_swaps(hash, base_asset, quote_asset)?


Why not just have this fucntion return Vec<ScheduledSwap> instead of Vec<(SwapLegInfo, BlockNumber)>?

Also then you could just delete SwapLegInfo and inline it into the ScheduledSwap structure, i.e. avoiding the serde(flatten).

I changed cf_scheduled_swaps to return Vec<ScheduledSwap> now, and simplified the code a bit.

However, I'm not sure merging the two structs is obviously justified. The reason I have separate structs is bacause get_scheduled_swap_legs doesn't deal with block numbers, so we'd need to pass the block number through that function just so it can construct ScheduledSwap on our behalf, which doesn't seem particularly nice. I would aslo need to add a generic parameter (for the block number) to ScheduledSwap to make it work. I don't think having this extra struct is much worse than the alternative. I can make this change if you have a strong preference though.

You should just inline get_scheduled_swap_legs, the fact that it causes you to need two struct's here (that aren't used separate other than in this one function), imo is clear indication the split isn't a good idea.

The advantage of having a separate get_scheduled_swap_legs is that we can write unit tests against it. It is possible to re-write tests with it inlined, but that wouldn't be as nice. I don't think having a separate struct for what the RPC call returns is such a bad thing, particularly since we wrap the internal type (with serde(flatten)), and so we don't even have any duplication.

AlastairHolmes · 2024-02-20T14:29:01Z

state-chain/custom-rpc/src/lib.rs

+	// swaps in the base_asset/quote_asset pool, including any "implicit" half-swaps (as a
+	// part of a swap involving two pools)
+	#[subscription(name = "subscribe_scheduled_swaps", item = SwapResponse)]
+	fn cf_subscribe_scheduled_swaps(&self, base_asset: Asset, quote_asset: Asset);


I suggest we add a cf_scheduled_swaps(base_asset, quote_asset, at) rpc too

Was a request of LPs, helps them know what as happened in the past

AlastairHolmes · 2024-02-20T14:51:55Z

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

+	pub from: Asset,
+	pub to: Asset,
+	pub amount: AssetAmount,
+	pub swap_type: SwapType,


Had a discussion with Kyle and Dan, we agreed that we don't want to expose the SwapType.

Also this means you don't need the serde derives on the SwapType enum, and can remove this member here

AlastairHolmes · 2024-02-20T15:09:34Z

state-chain/custom-rpc/src/lib.rs

+	// swaps in the base_asset/quote_asset pool, including any "implicit" half-swaps (as a
+	// part of a swap involving two pools)
+	#[subscription(name = "subscribe_scheduled_swaps", item = SwapResponse)]
+	fn cf_subscribe_scheduled_swaps(&self, base_asset: Asset, quote_asset: Asset);


This rpc call doesn't seem to have any checks on quote_asset? It should atleast fail, if it is not USDC, ideally I'd suggest just checking if the pool exists in cf_scheduled_swaps, and returning an error if it doesn't (In the pools pallet there is an error for this)

I reused cf_pool_info for this (creating a separate method returning true/false didn't seem worth it), however it doesn't seem trivial to return anything other than SubscriptionEmptyError for #[subscription] methods, so I'm just returning that. In practice this results in: {"jsonrpc":"2.0","error":{"code":-32602,"message":"Invalid params"},"id":1}.

AlastairHolmes · 2024-02-20T15:17:35Z

state-chain/runtime/src/lib.rs

+
+			debug_assert!(first_block < last_block);
+
+			(first_block..=last_block).flat_map(|block| {


You could just read the entire SwapQueue map into memory (Instead of one element at a time), then you can make this code much simpler.

AlastairHolmes · 2024-02-20T15:21:23Z

state-chain/runtime/src/lib.rs

+
+				Swapping::get_scheduled_swap_legs(swaps, base_asset).unwrap().into_iter().map(move |swap| (swap, block))
+			}).collect()
+


Suggested change

AlastairHolmes · 2024-02-20T15:37:38Z

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

 	pub(crate) type SwapQueue<T: Config> =
 		StorageMap<_, Twox64Concat, BlockNumberFor<T>, Vec<Swap>, ValueQuery>;

 	/// The first block for which swaps haven't yet been processed
 	#[pallet::storage]
+	#[pallet::getter(fn first_unprocessed_block)]


I'm not sure there is much point in using these getters really, particularly as you cannot update through a similarly named setter?

IMO it is a bit cleaner than writing pallet_cf_swapping::FirstUnprocessedBlock::<Runtime>::get(). I also don't think every getter needs a setter (in fact, it is a common incapsulation pattern to expose a value through a getter only so that it can only be modified internally).

Ok, I see your point. I hadn't seen that we do follow this pattern consistently elsewhere.

But if we do what I suggest inregards to the code that uses this getter, we wouldn't need to read this storage item anyway (Which isn't an exactly a nice concept (FirstUnprocessedBlock) to expose externally). And therefore the getter would have no purpose.

AlastairHolmes · 2024-02-20T15:48:55Z

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

+							swap_id: swap.swap_id,
+							from: swap.from,
+							// All swaps from `base_asset` have to go through Usdc:
+							to: Asset::Usdc,


Can you use the STABLE_ASSET constant? Same below.

AlastairHolmes

Once all these are addressed we can merge.

AlastairHolmes · 2024-02-21T10:04:17Z

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

 pub enum SwapType {
 	Swap(ForeignChainAddress),
 	CcmPrincipal(SwapId),
 	CcmGas(SwapId),
 }
 #[derive(Clone, Debug, PartialEq, Eq, Encode, Decode, TypeInfo, MaxEncodedLen)]
+#[cfg_attr(feature = "std", derive(serde::Serialize, serde::Deserialize))]


Sorry forgot to mention this. This serde derive is not used, and so can be removed.

AlastairHolmes · 2024-02-21T13:37:34Z

state-chain/custom-rpc/src/lib.rs

+		sink: SubscriptionSink,
+		base_asset: Asset,
+		quote_asset: Asset,
+	) -> Result<(), SubscriptionEmptyError> {


I've realised there is something else we need to deal with, swaps can fail will limited liquidity, so the simulation of the first leg can fail. I think what we should do is if the simulation fails, don't output the second leg as scheduled, but do output the first leg as being scheduled.

In this case if the liquidity gets added for the first leg, since the LPs can see that it requires liquidity, then it'll be swapped, but then the no one has accounted for the second leg? that seems bad? Would it not be better to use some kind approximate price for the first leg, to the estimate the second leg input - and perhaps making it clear through the API that this second amount is an estimate or something

This start to get a little messy with us mixing actual swaps with approximated swaps and now occasionally not including certain swaps. @dandanlen raised a concern about this to me, have you had a chance to chat about this in the office with him?

So I had a chat with Dan, I think the right way to deal with this is to instead of just having the estimate_amount via simulation is to also list the source_asset and source_amount, i.e. for multiple leg swaps what is the input asset and amount. And if the first leg simulation fails we can just set the estimated amount to zero?

Sure, I now include optional source_asset and source_amount for simulated swaps. Here is what it looks like for a BTC -> FLIP swap:

{ "swap_id":1, "base_asset":{"chain":"Ethereum","asset":"FLIP"}, "quote_asset":{"chain":"Ethereum","asset":"USDC"}, "side":"buy", "amount":492047747, "source_asset":{"chain":"Bitcoin","asset":"BTC"}, "source_amount":4949884, "execute_at":69 }

msgmaxim · 2024-02-22T06:30:05Z

Addressed/responded to all ouf your comments now @AlastairHolmes. Let me know is we can merge this (and whether your other comment re failing due to limited liquidity can be addressed separately).

AlastairHolmes · 2024-02-23T14:37:09Z

There is also another bug, the subscription can list swaps as being scheduled in past blocks, we can just bound the scheduled block to after the current block. Then I think this can be merged. This could happen when swaps fail, as swaps aren't rescheduled so their scheduled block doesn't change.

state-chain/runtime/src/lib.rs

…ero-liquidity * origin/main: fix: disable try-state checks (#4576) chore: debug solana in CI 🐛 (#4580) refactor: pass tx_ref as an extrinsic parameter (#4579) fix: remove bounded balance check (#4575) Solana: update image to latest tag (#4574) feat: add boost lp account to bouncer and fund it on setup_swaps (#4552) feat: Expose tx_hash on BroadcastSuccess event (#4561) feat: Relative Slippage Limits (PRO-1207) (#4547) chore: disable localnet solana in CI ⏱️ (#4569) feat: store prewitnessed deposits with id (#4496) Feat: Scheduled Swaps Subscription (#4525) # Conflicts: # state-chain/pallets/cf-pools/src/tests.rs

…utxo * origin/main: feat: expose command for broker fee withdrawal (#4581) Chore/fix arbitrum deployment (#4570) Added Range order pool price to the pool_price_v2 rpc call (#4548) fix: just check that the balance after is greater than before (#4587) chore: add origin to ccm failed (#4586) fix: runtime upgrade state check uses AllPalletsWithoutSystem (#4583) chore: add zellic audit to repo (#4585) fix: disable try-state checks (#4576) chore: debug solana in CI 🐛 (#4580) refactor: pass tx_ref as an extrinsic parameter (#4579) fix: remove bounded balance check (#4575) Solana: update image to latest tag (#4574) feat: add boost lp account to bouncer and fund it on setup_swaps (#4552) feat: Expose tx_hash on BroadcastSuccess event (#4561) feat: Relative Slippage Limits (PRO-1207) (#4547) chore: disable localnet solana in CI ⏱️ (#4569) feat: store prewitnessed deposits with id (#4496) Feat: Scheduled Swaps Subscription (#4525)

msgmaxim requested a review from kylezs February 14, 2024 07:52

msgmaxim requested a review from dandanlen as a code owner February 14, 2024 07:52

kylezs requested a review from AlastairHolmes February 14, 2024 07:57

feat: scheduled swaps subscription

f04027a

msgmaxim force-pushed the feat/scheduled-swaps-subscription branch from 5fbcd05 to f04027a Compare February 14, 2024 07:58

kylezs reviewed Feb 14, 2024

View reviewed changes

AlastairHolmes reviewed Feb 14, 2024

View reviewed changes

msgmaxim added 2 commits February 15, 2024 15:06

refactor: iterate over a range of block numbers

a0c1554

chore: address minor review comments

aa1fbb8

msgmaxim marked this pull request as draft February 16, 2024 11:11

feat: subscription also returns simulated implicit swaps

f142ef4

msgmaxim force-pushed the feat/scheduled-swaps-subscription branch from a00b549 to f142ef4 Compare February 19, 2024 00:39

Merge branch 'main' into feat/scheduled-swaps-subscription

e109324

msgmaxim force-pushed the feat/scheduled-swaps-subscription branch from 5593e95 to e109324 Compare February 19, 2024 05:01

msgmaxim marked this pull request as ready for review February 19, 2024 05:02

kylezs reviewed Feb 19, 2024

View reviewed changes

msgmaxim added 3 commits February 20, 2024 11:55

chore: address clippy

95ec9f2

chore: fix tests

a5e0be7

Merge branch 'main' into feat/scheduled-swaps-subscription

7440c54

AlastairHolmes reviewed Feb 20, 2024

View reviewed changes

msgmaxim added 2 commits February 21, 2024 15:09

refactor: cf_scheduled_swaps returns Vec<ScheduledSwap>

05489e7

feat: remove swap_type

19f7e11

AlastairHolmes reviewed Feb 21, 2024

View reviewed changes

msgmaxim added 9 commits February 22, 2024 12:18

feat: return error on invalid pool

b23220f

chore: use STABLE_ASSET constant

8962ae5

fix: cfg_attr for serde(flatten)

e779855

refactor: read entire swap queue

0e7b8e0

feat: add scheduled swaps rpc call

2c539dc

Merge branch 'main' into feat/scheduled-swaps-subscription

e435924

feat: use side in SwapLegInfo

81a1036

fix: correctly map error

e47f345

chore: format

160ebf7

fix: correctly show execute_at for re-tried swaps

626633c

AlastairHolmes reviewed Feb 26, 2024

View reviewed changes

state-chain/runtime/src/lib.rs Outdated Show resolved Hide resolved

msgmaxim added 2 commits February 26, 2024 22:37

refactor: use cmp::max

5af8fff

feat: source asset and amount for simulated swaps

ff96517

AlastairHolmes approved these changes Feb 26, 2024

View reviewed changes

Merge branch 'main' into feat/scheduled-swaps-subscription

a2ef796

msgmaxim enabled auto-merge (squash) February 27, 2024 03:26

msgmaxim merged commit 64f7b99 into main Feb 27, 2024
43 checks passed

msgmaxim deleted the feat/scheduled-swaps-subscription branch February 27, 2024 06:00


		debug_assert!(first_block < last_block);

		(first_block..=last_block).flat_map(\|block\| {


		Swapping::get_scheduled_swap_legs(swaps, base_asset).unwrap().into_iter().map(move \|swap\| (swap, block))
		}).collect()

Feat: Scheduled Swaps Subscription #4525

Feat: Scheduled Swaps Subscription #4525

Conversation

msgmaxim commented Feb 14, 2024

Pull Request

Checklist

Summary

codecov bot commented Feb 14, 2024 • edited Loading

Codecov Report

kylezs commented Feb 14, 2024

kylezs commented Feb 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlastairHolmes Feb 14, 2024 • edited Loading

Choose a reason for hiding this comment

AlastairHolmes commented Feb 14, 2024

kylezs left a comment

Choose a reason for hiding this comment

AlastairHolmes Feb 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlastairHolmes Feb 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlastairHolmes Feb 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlastairHolmes Feb 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msgmaxim Feb 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msgmaxim Feb 21, 2024 • edited Loading

Choose a reason for hiding this comment

AlastairHolmes Feb 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlastairHolmes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlastairHolmes Feb 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlastairHolmes Feb 23, 2024 • edited Loading

Choose a reason for hiding this comment

msgmaxim Feb 26, 2024 • edited Loading

Choose a reason for hiding this comment

msgmaxim commented Feb 22, 2024

AlastairHolmes commented Feb 23, 2024

codecov bot commented Feb 14, 2024 •

edited

Loading

AlastairHolmes Feb 14, 2024 •

edited

Loading

AlastairHolmes Feb 20, 2024 •

edited

Loading

AlastairHolmes Feb 20, 2024 •

edited

Loading

AlastairHolmes Feb 20, 2024 •

edited

Loading

AlastairHolmes Feb 20, 2024 •

edited

Loading

msgmaxim Feb 21, 2024 •

edited

Loading

msgmaxim Feb 21, 2024 •

edited

Loading

AlastairHolmes Feb 21, 2024 •

edited

Loading

AlastairHolmes Feb 21, 2024 •

edited

Loading

AlastairHolmes Feb 23, 2024 •

edited

Loading

msgmaxim Feb 26, 2024 •

edited

Loading