feat: remove swap and retry batch on price impact #5285

msgmaxim · 2024-09-20T05:55:26Z

Pull Request

Checklist

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

I am confident that the code works.
I have written sufficient tests.
I have written and tested required migrations.
I have updated documentation where appropriate.

Summary

Moved the retry loop from on_finalize (where we only retried on FoK failures) to execute_batch (where now we retry on both price impact and FoK failures). In order to determine which swap to remove in case of "price impact" we compare swap_amount (which may be input amount, or may be the result of the first leg) and remove the largest; for this I added swap_states to SwapLegFailed.

One thing to note is that now BatchSwapFailed may be emitted more than once per block due to retrying. I think it makes sense: each error would refer to a different batch, potentially to different pools even, but it also might not be ideal if the same error is repeated many times with consequent errors not providing much additional information.

dandanlen

Looks good - I want to take another look on Monday but I don't expect any major comments.

dandanlen

I was wrong - I have some comments :)

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

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

codecov · 2024-09-25T03:04:56Z

Codecov Report

Attention: Patch coverage is 93.97260% with 22 lines in your changes missing coverage. Please review.

Project coverage is 70%. Comparing base (fa72c49) to head (5c964e4).
Report is 6 commits behind head on main.

Files with missing lines	Patch %	Lines
state-chain/pallets/cf-swapping/src/lib.rs	86%	14 Missing ⚠️
state-chain/amm/src/limit_orders.rs	0%	0 Missing and 3 partials ⚠️
state-chain/amm/src/range_orders.rs	0%	0 Missing and 3 partials ⚠️
state-chain/amm/src/lib.rs	0%	0 Missing and 1 partial ⚠️
state-chain/cf-integration-tests/src/swapping.rs	98%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@          Coverage Diff          @@
##            main   #5285   +/-   ##
=====================================
- Coverage     70%     70%   -0%     
=====================================
  Files        487     487           
  Lines      87463   87489   +26     
  Branches   87463   87489   +26     
=====================================
- Hits       61509   61450   -59     
- Misses     22672   22755   +83     
- Partials    3282    3284    +2

Flag	Coverage Δ
	`70% <94%> (-<1%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

dandanlen

Looking good now. I'll give it another pass and try to get it merged tomorrow.

dandanlen · 2024-09-26T10:01:49Z

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

+						Self::deposit_event(Event::<T>::BatchSwapFailed {
+							asset,
+							direction,
+							amount,
+						});


It might we worth adjusting how we communicate this: the swaps have not longer actually failed, they might be retried. Maybe we don't need this event at all any more?

I also thought about deleting this. I think this was only used for alterts, but because now these failures no longer block other swaps, this is really no different from "low liquidity" (for which we already have other alerts, I think).

dandanlen · 2024-09-26T10:01:52Z

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

+						// Abort if we couldn't find a swap to remove; otherwise, remove the
+						// swap and try again with the remaining swaps:


This comment is not quite accurate any more / a bit confusing.

Combined with the expression above and updated the comment.

dandanlen · 2024-09-26T10:31:12Z

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

+						failed_swaps.extend(violating_swaps);
+						swaps_to_execute = non_violating_swaps;
+					},
+					Err(BatchExecutionError::DispatchError { error }) => return Err(error),


This is the only error condition in this loop, and it can only happen if we exceed the the transaction depth limit, right? So am I right in thinking that this function (execute_batch) doesn't need to be transactional (since it can never really error, and never needs to roll back)?

ie. instead of passing this error back up the chain, we could just log it at this level.

Right, I changed this to never return BatchExecutionOutcomes directly now.

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

dandanlen · 2024-09-26T10:38:17Z

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

+	pub fn add_liquidity(asset: Asset, amount: AssetAmount) {
+		let liquidity = Liquidity::get()


I was thinking there might be a simpler way to simulate this. We don't need to add amounts etc, we could just configure the mock to behave as if there is no liquidity in a certain pool (ie. instead of a liquidity amount we just have a bool).

This would avoid having to calculate/reason about how much input is required at current prices to make the test fail etc.

I almost ended up making this change, but then ran into the price_impact_removes_one_swap test where we actually want low liquidity, but enough for one swap to test the retrying works (and even the swaps from the failing pool/side can be retired). Because retrying is done entirely within a single call to on_finalize there is no way for us to change liquidity between iteration as we remove swaps, so we need a bit more control in the mock's liquidity than to just say "all swaps in the pool should fail".

Also, I thought it was nice that we could check the resulting liquidity in presence of rollbacks (e.g. in storage_state_rolls_back_on_fok_violation), which I thought was the whole point of marking Liquidity as storage.

dandanlen

I thought we wanted to remove the BatchSwapFailed event?

state-chain/cf-integration-tests/src/swapping.rs

msgmaxim · 2024-09-30T02:16:57Z

I thought we wanted to remove the BatchSwapFailed event?

Maybe. Though it also sounded like we wanted to at least replace it with another way to tell that a swap failed due to low liquidity (for example, by adding a field to SwapRescheduled). It seemed more appropriate to leave it for another PR.

dandanlen · 2024-09-30T09:16:37Z

Ok I'll open an issue to follow this up.

(PRO-1692)

…-sdk-1.15.2 * origin/main: (31 commits) feat: liveness electoral system (#5278) chore: bump asset-balances to match release (#5305) refactor: Strongly-typed identifiers for SwapId/SwapRequestId (#5294) fix: only install solana if `run-job` is true 🐛 (#5304) Chore/debug 3 nodes (#5302) feat: remove swap and retry batch on price impact (#5285) refactor: Collect settings extrinsic in threshold signer pallet into a single extrinsic (#5299) feat: add solana monitoring data (#5277) fix: Redemption amount printed not consistent in rounding (#5296) refactor: use chainflip api for DCA test (#5289) fix: add audit exception for RUSTSEC-2024-0375 (#5303) fix: filter out stale bitcoin utxos (#5291) PRO-1594: Add healthcheck endpoints to broker and lp apis (#5282) Tests elections pallet (#5190) PRO-1620: Ensure that default port is used if none is given in configuration (#5281) test: egress success tests (#5288) feat: localnet scripts to create and recreate easier (#5284) Denote broadcast timeout in target chain blocks. (#5270) fix: force version bump endpoint (#5280) refactor: user friendly bouncer new swap cmd (#5273) ... # Conflicts: # .zepter.yaml # Cargo.lock # api/bin/chainflip-cli/Cargo.toml # api/bin/chainflip-lp-api/src/main.rs # engine/Cargo.toml # state-chain/custom-rpc/Cargo.toml # state-chain/custom-rpc/src/lib.rs # state-chain/pallets/cf-swapping/src/lib.rs # state-chain/runtime/src/monitoring_apis.rs # utilities/Cargo.toml

* feat: remove swap and retry batch on price impact * fix: storage rolls back on FoK violation * feat: use storage for liquidity mock * refactor: split_off_highest_impact_swap * chore: remove unnecessary tests * test: rework failed_swaps_are_rolled_back integration test * refactor: use add_liquidity in setup_pool_and_accounts * refactor: execute_batch never to return Error * chore: update comment * test: check full state of liquidity pools

msgmaxim requested a review from dandanlen as a code owner September 20, 2024 05:55

feat: remove swap and retry batch on price impact

5612077

msgmaxim force-pushed the feat/swap-batching branch from 7035c1b to 5612077 Compare September 20, 2024 07:38

dandanlen reviewed Sep 20, 2024

View reviewed changes

dandanlen reviewed Sep 23, 2024

View reviewed changes

msgmaxim added 6 commits September 24, 2024 12:11

fix: storage rolls back on FoK violation

8f75765

feat: use storage for liquidity mock

52248f2

refactor: split_off_highest_impact_swap

dc53973

Merge branch 'main' into feat/swap-batching

5163414

chore: remove unnecessary tests

c40156a

test: rework failed_swaps_are_rolled_back integration test

6d69925

refactor: use add_liquidity in setup_pool_and_accounts

d23036a

dandanlen reviewed Sep 26, 2024

View reviewed changes

msgmaxim added 3 commits September 27, 2024 11:49

refactor: execute_batch never to return Error

429e42c

chore: update comment

72e7e6f

Merge branch 'main' into feat/swap-batching

6a7d3bd

dandanlen reviewed Sep 27, 2024

View reviewed changes

state-chain/cf-integration-tests/src/swapping.rs Outdated Show resolved Hide resolved

test: check full state of liquidity pools

5c964e4

dandanlen approved these changes Sep 30, 2024

View reviewed changes

dandanlen added this pull request to the merge queue Sep 30, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 30, 2024

dandanlen added this pull request to the merge queue Sep 30, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 30, 2024

msgmaxim added this pull request to the merge queue Oct 1, 2024

Merged via the queue into main with commit ec6c5b7 Oct 1, 2024
49 checks passed

msgmaxim deleted the feat/swap-batching branch October 1, 2024 06:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: remove swap and retry batch on price impact #5285

feat: remove swap and retry batch on price impact #5285

msgmaxim commented Sep 20, 2024 •

edited

Loading

dandanlen left a comment

dandanlen left a comment

codecov bot commented Sep 25, 2024 •

edited

Loading

dandanlen left a comment

dandanlen Sep 26, 2024

msgmaxim Sep 26, 2024

dandanlen Sep 26, 2024

msgmaxim Sep 27, 2024

dandanlen Sep 26, 2024

msgmaxim Sep 27, 2024

dandanlen Sep 26, 2024

msgmaxim Sep 27, 2024

dandanlen left a comment

msgmaxim commented Sep 30, 2024

dandanlen commented Sep 30, 2024 •

edited

Loading

		// Abort if we couldn't find a swap to remove; otherwise, remove the
		// swap and try again with the remaining swaps:

		pub fn add_liquidity(asset: Asset, amount: AssetAmount) {
		let liquidity = Liquidity::get()

feat: remove swap and retry batch on price impact #5285

feat: remove swap and retry batch on price impact #5285

Conversation

msgmaxim commented Sep 20, 2024 • edited Loading

Pull Request

Checklist

Summary

dandanlen left a comment

Choose a reason for hiding this comment

dandanlen left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 25, 2024 • edited Loading

Codecov Report

dandanlen left a 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

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

dandanlen left a comment

Choose a reason for hiding this comment

msgmaxim commented Sep 30, 2024

dandanlen commented Sep 30, 2024 • edited Loading

msgmaxim commented Sep 20, 2024 •

edited

Loading

codecov bot commented Sep 25, 2024 •

edited

Loading

dandanlen commented Sep 30, 2024 •

edited

Loading