fix: DOT swap output less than existential deposit #4062

marcellorigotti · 2023-09-28T15:10:13Z

Pull Request

Closes: PRO-871

Issue to resolve comments about the test: PRO-926

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

This test perform 2 swaps:

the first one 5 usdc for less than 1 dot, resulting in an output < Existential deposit, the transaction should be still broadcasted successfully, but the balance of the destination address remain 0.00
the second swap is a normal USDC -> DOT swap to showcase that the resulting balance is correctly increased after the swap

Dot egress fix:

For dot egress, Instead of just looking at extrinsics with transfer events, we look at all extrinsics with ExtrinsicSuccess

linear · 2023-09-28T15:10:15Z

PRO-814 Handle egress below existential deposit

It seems like we still don't completely handle this case. AFAIK we use force transfer to ensure the legitimate transfers go through, even if one of them cannot because it's below the ED. However, it seems we're still expecting to witness the success of the egress, which of course, cannot occur. So we will still see a broadcast aborted, despite the transaction succeeding.

alastair saw this on his localnet.

codecov · 2023-09-28T15:21:38Z

Codecov Report

Merging #4062 (a504bd3) into main (03c4f3c) will increase coverage by 0%.
Report is 13 commits behind head on main.
The diff coverage is 53%.

@@          Coverage Diff           @@
##            main   #4062    +/-   ##
======================================
  Coverage     72%     72%            
======================================
  Files        378     378            
  Lines      60663   60906   +243     
  Branches   60663   60906   +243     
======================================
+ Hits       43495   43677   +182     
- Misses     14904   14964    +60     
- Partials    2264    2265     +1

Files	Coverage Δ
engine/src/witness/dot/dot_deposits.rs	`60% <75%> (-1%)`	⬇️
engine/src/witness/dot.rs	`29% <48%> (+5%)`	⬆️

... and 16 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

marcellorigotti · 2023-09-28T15:35:56Z

Bouncer will never complete because of the test I just added.
When doing a single swap containing only a <ED dot as output we never emit the event BroadcastSuccess, hence the test will loop endlessly waiting for the event.

linear · 2023-10-18T02:35:14Z

PRO-871 Fix Polkadot broadcast success witnessing

Problem

We can currently ignore polkadot broadcast successes if they don't emit a Transfer event.

If we make a force_batch tx containing a single transfer that is below the existential deposit, the broadcast succeeds, since force_batch succeeds regardless of if the internal txs succeed or not. But since the inner transfer doesn't emit a transfer event - because it didn't succeed, we ignore it.

Current code break down

The current dot witnessing code collects extrinsic indices, which are the extrinsic indices of that block, which produced events that are relevant to us. In the dot_depositsadapter not that we check that we received a Transfer event, and that

				if &PolkadotAccountId::from_aliased(from.0) == our_vault ||
					&deposit_address == our_vault

This if statement checks:

We have received something from our vault. This is currently capturing egress transactions to other dot addresses.
The deposit address is our vault. This is the case when we fetch funds from our generated addresses that have received deposits into our root/"vault" account.

Solution

For both of these we could instead use the ProxyExeucted event - and this would of course be done in a separate component not as part of the Deposit capturing. Currently the responsibilities are a bit blurred, so this should help fix that too.

Since all transactions we create are via the proxy, and we get a ProxyExecuted event from our vault, we can use this is broadcast success. This is conceptually similar to what we do to witness Ethereum broadcast successes, since we use the SignatureAcceptedevent from the KeyManager contract.

For the broadcast to be registered as success, we must submit the transaction_succeeed extrinsic.

To test this works, of course all the current bouncer tests should work, and the test in PRO-814 should pass too. That PR can be rebased onto this one in order to test it (CC: marcello - whoever picks this up might take over that branch).

Alternative Solutions Considered

Use the list of TxOutIds to filter the extrinsics we are watching, and then filter only the events from these extrinsics.

The drawback of this might be that we would not witness events that are triggered independently (for example governance intervention).

kylezs

In the interests of getting this in quickly. I'm happy if the code comments are resolved, and an issue is created about the test changes

kylezs · 2023-10-18T07:00:33Z

bouncer/run.sh

@@ -2,6 +2,7 @@ set -e
 ./commands/observe_block.ts 5
 ./commands/setup_vaults.ts
 ./commands/setup_swaps.ts
+./tests/swap_less_than_existential_deposit_dot.ts


we should put this as part of the concurrent tests - though, see comment further down

kylezs · 2023-10-18T07:03:25Z

bouncer/tests/swap_less_than_existential_deposit_dot.ts

+  runWithTimeout,
+} from '../shared/utils';
+
+// This code is duplicated to allow us to specify a specific amount we want to swap


Why is it duplicated? We should factor out the common code here.

Because I needed a custom version of the function where I could specify the amount to swap to be sure the output was less than ED, and where the result was determined by observing broadcastSuccess instead of balanceUpdated.

Sure, it's not all common, but we can factor out the common code, potentially have one function where you can optionally provide an event to observe - haven't thought deeply about how exactly to do it, but it can be done

testSwap, performSwap and doPerformSwap have "amount" arguments as of commit 0a09832

kylezs · 2023-10-18T07:05:19Z

bouncer/tests/swap_less_than_existential_deposit_dot.ts

+  amount: string,
+  balanceIncrease: boolean,
+  tag = '',
+  messageMetadata?: CcmDepositMetadata,


this isn't used - deduplicating ofc fixes this too

kylezs · 2023-10-18T07:05:58Z

bouncer/tests/swap_less_than_existential_deposit_dot.ts

+// and to wait for some specific events
+export async function doPerformSwap(
+  { sourceAsset, destAsset, destAddress, depositAddress, channelId }: SwapParams,
+  amount: string,


why string and not number?

no reason sendERC20 uses string so I just used it as well, we can change it if it makes things more clear

kylezs · 2023-10-18T07:07:32Z

bouncer/tests/swap_less_than_existential_deposit_dot.ts

+  let stopObserving = false;
+  const observingBadEvents = observeBadEvents(':BroadcastAborted', () => stopObserving);
+
+  // The initial price is 10USDC = 1DOT,


How do we know this? This test is going to be quite flakey with hardcoded values. Ideally we query for the price, and then swap an amount that results in an egress less than the ED.

This might mean this test can't be part of concurrent tests, but that's fine, would rather have a test that when run individually we know is correct, regardless of the state of the pools at any point in time.

Yeah that's why I put this test as first, because I knew the states of the pool at the beginning of the bouncer (I wasn't sure how to query the prices but maybe it is easy to do so?), and that's also why it wasn't inside the concurrent tests, to be sure that when this is executed the price doesn't suddenly change with another swap.

It might not be easy to do, to check the price, but
a) this is a more useful / robust if we do
b) being able to query for the price will be useful for other more complex / granular swapping tests in the future
c) we can test the relevant price APIs too

so I think it's worth doing, but as mentioned, since I'm unsure how long it'll take I don't mind merging this as is, since the test passed in this order, and then fixing this as part of another issue.

If we can query the price of the pool and move this in the concurrent tests then we should also update the observeEvent call to be sure to observe the correct polkadotBrodcast:
I guess we can easily derive the broadcastID knowing the swap channel, right?

sorry replied before reading your answer, anyway I'll create a linear ticket with all this info so we don't forget if we merge this!

engine/src/witness/dot/dot_deposits.rs

engine/src/witness/dot.rs

swap (output less than ED of DOT) test to bouncer

d4e57f5

fix lint

380bc43

throw error in case we observe BroadcastFailure

070ccf1

marcellorigotti force-pushed the feature/pro-814 branch from 9d7b5cf to a59c65c Compare September 29, 2023 09:43

fix lint

6e4dd8b

marcellorigotti force-pushed the feature/pro-814 branch from a59c65c to 6e4dd8b Compare September 29, 2023 11:36

Added timeout in case no good/bad event is received

ef4a68a

marcellorigotti force-pushed the feature/pro-814 branch from bda8115 to ef4a68a Compare October 5, 2023 11:44

Merge branch 'main' into feature/pro-814

bfb94e7

j4m1ef0rd self-assigned this Oct 17, 2023

j4m1ef0rd force-pushed the feature/pro-814 branch from 0c7fd7c to 44a3b3e Compare October 18, 2023 02:32

j4m1ef0rd requested a review from kylezs October 18, 2023 02:33

j4m1ef0rd added CFE Bouncer labels Oct 18, 2023

j4m1ef0rd force-pushed the feature/pro-814 branch from 44a3b3e to f8b2d2b Compare October 18, 2023 02:34

j4m1ef0rd added 2 commits October 18, 2023 13:50

fix: use extrinsic success event for dot egress

f8fc34a

fix: increase bouncer timeout

5c46326

j4m1ef0rd force-pushed the feature/pro-814 branch from f8b2d2b to 5c46326 Compare October 18, 2023 02:50

j4m1ef0rd marked this pull request as ready for review October 18, 2023 06:03

j4m1ef0rd requested a review from martin-chainflip as a code owner October 18, 2023 06:03

kylezs changed the title ~~swap (output less than ED of DOT) test to bouncer~~ fix: DOT swap output less than existential deposit Oct 18, 2023

kylezs reviewed Oct 18, 2023

View reviewed changes

refactor: move out of deposit witnessing

48e7432

j4m1ef0rd requested a review from kylezs October 19, 2023 02:58

chore: some cosmetic changes

a504bd3

kylezs approved these changes Oct 19, 2023

View reviewed changes

kylezs enabled auto-merge (squash) October 19, 2023 08:49

kylezs merged commit abb0f48 into main Oct 19, 2023

kylezs deleted the feature/pro-814 branch October 19, 2023 09:22

linear bot mentioned this pull request Nov 2, 2023

feat: improve Dot Existential Deposit Test #4195

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: DOT swap output less than existential deposit #4062

fix: DOT swap output less than existential deposit #4062

marcellorigotti commented Sep 28, 2023 •

edited by kylezs

Loading

linear bot commented Sep 28, 2023

codecov bot commented Sep 28, 2023 •

edited

Loading

marcellorigotti commented Sep 28, 2023

linear bot commented Oct 18, 2023

Problem

Current code break down

Solution

Alternative Solutions Considered

kylezs left a comment

kylezs Oct 18, 2023

kylezs Oct 18, 2023

marcellorigotti Oct 18, 2023

kylezs Oct 18, 2023 •

edited

Loading

martin-chainflip Oct 18, 2023

kylezs Oct 18, 2023

kylezs Oct 18, 2023

marcellorigotti Oct 18, 2023

kylezs Oct 18, 2023

marcellorigotti Oct 18, 2023

kylezs Oct 18, 2023

marcellorigotti Oct 18, 2023 •

edited

Loading

marcellorigotti Oct 18, 2023

fix: DOT swap output less than existential deposit #4062

fix: DOT swap output less than existential deposit #4062

Conversation

marcellorigotti commented Sep 28, 2023 • edited by kylezs Loading

Pull Request

Checklist

Summary

linear bot commented Sep 28, 2023

codecov bot commented Sep 28, 2023 • edited Loading

Codecov Report

marcellorigotti commented Sep 28, 2023

linear bot commented Oct 18, 2023

Problem

Current code break down

Solution

Alternative Solutions Considered

kylezs 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

kylezs Oct 18, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcellorigotti Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcellorigotti commented Sep 28, 2023 •

edited by kylezs

Loading

codecov bot commented Sep 28, 2023 •

edited

Loading

kylezs Oct 18, 2023 •

edited

Loading

marcellorigotti Oct 18, 2023 •

edited

Loading