FI: Add swapping events #1764

lemunozm · 2024-03-08T12:08:35Z

Description

Add SwapCreated and SwapFullfilled events from the PoV of FI.

Should be merged before RU

lemunozm

@hieronx, I think the events are complete, but tell me if you think some information is missing in the events.

lemunozm · 2024-03-08T12:09:56Z

pallets/foreign-investments/src/impls.rs

+				remaining: Swap {
+					amount_out: status.pending,
+					..swap.clone()
+				},


Note that ..swap.clone() contains the currency_in and currency_out to know the swap direction.

wischli

Quick like a spaceship! One comment regarding a potential redundancy. Moreover, it would be nice to assert the newly added events in a unit test.

wischli · 2024-03-08T14:57:18Z

libs/traits/src/swaps.rs

+	/// Ratio used to swap `swapped_out` into `swapped_in`
+	pub ratio: Ratio,


The OrderInfo returned by TokenSwaps::get_order_details contains a ratio field of type OrderRatio<Ratio>. Maybe that suffices such that we don't need to introduce an additional ratio field?

OrderRatio is the ratio you want to configure: the way the order behaves. The ratio here is the ratio value itself. It differs, i.e., when OrderRatio::Market, we do not know the ratio value.

Thanks for the explanation! Makes sense to keep as is then.

lemunozm · 2024-03-08T16:22:37Z

Moreover, it would be nice to assert the newly added events in a unit test.

Agree! I'll do it in a follow-up PR to not block this in case I have no time to finish them

lemunozm · 2024-03-08T17:47:00Z

Modified to return always the market ratio in a fulfilled event even when it comes from a cancelation.

wischli

LGTM!

lemunozm · 2024-03-11T08:57:32Z

We should not merge it yet. There are some requested changes from slack

lemunozm · 2024-03-11T10:42:26Z

pallets/foreign-investments/src/lib.rs

+	#[pallet::event]
+	#[pallet::generate_deposit(pub(super) fn deposit_event)]
+	pub enum Event<T: Config> {
+		// The swap is created and now is wating to be fulfilled
+		SwapCreated {
+			who: T::AccountId,
+			swap_id: SwapId<T>,
+			swap: SwapOf<T>,
+		},
+		// The swap was fulfilled by another participant.
+		SwapFullfilled {
+			who: T::AccountId,
+			swap_id: SwapId<T>,
+			remaining: SwapOf<T>,
+			swapped_in: T::SwapBalance,
+			swapped_out: T::SwapBalance,
+		},
+		// The swap was fulfilled by cancelling an opposite swap for the same foreign investment.
+		SwapCancelled {
+			who: T::AccountId,
+			swap_id: SwapId<T>,
+			remaining: SwapOf<T>,
+			cancelled_in: T::SwapBalance,
+			opposite_in: T::SwapBalance,
+		},
+	}


@hieronx I've needed to add a new SwapCancelled. All events are seen from the perspective of who did the swap.

Wouldn’t it possible to add an optional arg to the decrease fulfilled event, with the swap ID that was used to cancel it out?

This can not be possible due that swap ID is the same for both directions. Note that swap id is not the order id, which is unknown from FI perspective. As a workaround, I've added the opposite_in field to SwapCancelled, which has the amount in the opposite direction that has not been canceled. That way, if opposite_in is 0, that means that the cancellation has also canceled the opposite order. That is what you need to know.

Looks good to me! Only one nit: in SwapFulfilled, wouldn't it make more sense to have the swapped_out param before the swapped_in param?

What do you mean by "before" in this context? Placed in swapped line numbers? Probably, it makes more sense. But right now, we have such conventions in all places in the codebase. I'd prefer to keep it as it is everywhere (first param in, second param out). But we should probably look into it soon 👍🏻

Well we should never change these event definitions again after merging this, at least not without a very strong reason ;) Would be a pain on the subquery side. But sure it is fine to keep it as is then if that is the convention now

mustermeiszer

Just RC as I want to see before merge

wischli · 2024-03-11T18:28:30Z

Just RC as I want to see before merge

Just triggered a WASM generation and can update Dev later.

mustermeiszer

Code looks good!

I am still a bit confused to follow the reasoning of the code, but the last time I had the same and found no issues. What would help me if @lemunozm could describe what are the possible states after apply_swap, how are they reflected in the SwapStatus and the SwapOf.

mustermeiszer · 2024-03-12T09:34:16Z

pallets/foreign-investments/src/impls.rs

+		if !status.swapped.is_zero() {
+			Pallet::<T>::deposit_event(Event::SwapCancelled {
+				who: who.clone(),
+				swap_id,
+				remaining: Swap {
+					amount_out: status.pending,
+					..swap.clone()
+				},
+				cancelled_in: status.swapped,
+				opposite_in: T::Swaps::pending_amount(who, swap_id, swap.currency_in)?,
+			});
+		}
+
+		if !status.pending.is_zero() {
+			Pallet::<T>::deposit_event(Event::SwapCreated {
+				who: who.clone(),
+				swap_id,
+				swap: Swap {
+					amount_out: status.pending,
+					..swap.clone()
+				},
+			})
+		}


Could it be that both status.swapped and status.pending are non zero?

IF YES:

How would one interpret the swap_id of the two events?

Yes, it could. swap_id will be the same for both. Note that swap_id is just a tuple (T::InvestmentId, Action) not an order_id (which is a number from order-book).

If the above two events are dispatched, it means that when you apply a swap, part of the amount was automatically fulfilled by cancelation with an inverse order (notified by SwapCancelled event), and part of the swap could not be fulfilled with the inverse order: such remaining amount needs to be swapped (notified it by SwapCreated event).

Possible states after applying swaps are:

pending_amount == 0 and swapped amount > 0 => it means applying a swap has partially or fully canceled any previous inverse swaps. The required amount is obtained from the inverse amount. No more amount needs to be swapped. OrderBook will Not create a new order.

pending amount > 0 and swapped amount > 0 => It means applying a swap has fully canceled any previous inverse swaps, but more amount needs to be swapped. Orderbook will create a new order.

pending amount > 0 and swapped amount == 0 => There was no inverse order, so we need to create a new order.

NOTE: Probably, a better name for swap_status.swapped is swap_status.canceled. It all depends on how you interpret it.

Thanks a lot, that helped!

lemunozm added 2 commits March 8, 2024 13:01

adding swap events in FI

f40c31a

update runtimes

a4c781d

lemunozm requested review from hieronx, wischli and mustermeiszer March 8, 2024 12:08

lemunozm commented Mar 8, 2024

View reviewed changes

lemunozm added 2 commits March 8, 2024 13:13

fix runtimes

c34db97

fix clippy

22e94d9

wischli reviewed Mar 8, 2024

View reviewed changes

lemunozm added 2 commits March 8, 2024 18:35

return always market ratio

941c19c

wire to one if both currencies are the same

44e962e

Merge branch 'main' into fi/swap-events

4ccdf67

wischli previously approved these changes Mar 11, 2024

View reviewed changes

remove ratio from FI events

c86da3c

lemunozm dismissed wischli’s stale review via c86da3c March 11, 2024 10:35

minor rename

6d4215f

lemunozm commented Mar 11, 2024

View reviewed changes

mustermeiszer requested changes Mar 11, 2024

View reviewed changes

wischli temporarily deployed to production March 11, 2024 18:27 — with GitHub Actions Inactive

mustermeiszer reviewed Mar 12, 2024

View reviewed changes

mustermeiszer approved these changes Mar 12, 2024

View reviewed changes

wischli approved these changes Mar 12, 2024

View reviewed changes

lemunozm merged commit cd1d36d into main Mar 12, 2024
12 checks passed

lemunozm deleted the fi/swap-events branch March 12, 2024 11:53

wischli mentioned this pull request Mar 12, 2024

Centrifuge 1026 #1768

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FI: Add swapping events #1764

FI: Add swapping events #1764

lemunozm commented Mar 8, 2024

lemunozm left a comment •

edited

lemunozm Mar 8, 2024

wischli left a comment

wischli Mar 8, 2024

lemunozm Mar 8, 2024

wischli Mar 11, 2024

lemunozm commented Mar 8, 2024

lemunozm commented Mar 8, 2024

wischli left a comment

lemunozm commented Mar 11, 2024

lemunozm Mar 11, 2024 •

edited

hieronx Mar 11, 2024

lemunozm Mar 11, 2024 •

edited

hieronx Mar 11, 2024

mustermeiszer left a comment

wischli commented Mar 11, 2024

mustermeiszer left a comment

mustermeiszer Mar 12, 2024

lemunozm Mar 12, 2024

mustermeiszer Mar 12, 2024

		/// Ratio used to swap `swapped_out` into `swapped_in`
		pub ratio: Ratio,

FI: Add swapping events #1764

FI: Add swapping events #1764

Conversation

lemunozm commented Mar 8, 2024

Description

lemunozm left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wischli 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

lemunozm commented Mar 8, 2024

lemunozm commented Mar 8, 2024

wischli left a comment

Choose a reason for hiding this comment

lemunozm commented Mar 11, 2024

lemunozm Mar 11, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lemunozm Mar 11, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mustermeiszer left a comment

Choose a reason for hiding this comment

wischli commented Mar 11, 2024

mustermeiszer 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

lemunozm left a comment •

edited

lemunozm Mar 11, 2024 •

edited

lemunozm Mar 11, 2024 •

edited