feat: update custom_rpc, runtime_api and broker api for broker level screening #5341

MxmUrw · 2024-10-21T13:50:20Z

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

In this PR all changes to the broker API which are required for broker level screening are going to be merged. Currently based on @Janislav 's PR ~~(#5310)~~ (#5332) which implements the extrinsic for rejecting transactions.

Once that PR is merged, rebase this on main.

This PR includes the following changes:

Add new methods to custom_rpc:
- cf_subscribe_tainted_btc_transaction_events which is a subscription for all events related with tainted transactions occuring in the ingress egress pallet.
- cf_open_btc_deposit_channels which returns all open channel addresses for a given account id.
These methods forward to corresponding implementations in the runtime api.
Changes to the broker API:
- Add mark_btc_transaction_as_tainted endpoint which submits the corresponding extrinsic.
- Add open_btc_deposit_channels method.
- Add subscribe_tainted_btc_transaction_events method which forwards the node subscription of the same name.

Non-Breaking changes

If this PR includes non-breaking changes, select the non-breaking label. On merge, CI will automatically cherry-pick the commit to a PR against the release branch.

MxmUrw · 2024-10-29T16:05:05Z

To be discussed: error handling in the subscription forwarding in the broker api (subscribe_tainted_btc_transaction_events). Currently a bit ad-hoc, forwarding a Result<_,String> whose error-variant is used for the case where the subscription message from the engine cannot be decoded.

commit 3166701 Author: Maxim Urschumzew <maxu@chainflip.io> Date: Tue Oct 29 15:27:51 2024 +0100 Add `subscribe_tainted_btc_transaction_events` method to Broker API. It which forwards the node subscription of the same name. commit 010772d Author: Maxim Urschumzew <maxu@chainflip.io> Date: Tue Oct 29 13:56:07 2024 +0100 Add `open_btc_deposit_channels` method to broker api. commit 9ea7e6f Author: Maxim Urschumzew <maxu@chainflip.io> Date: Tue Oct 29 11:19:07 2024 +0100 Add subscription endpoint for tainted transaction events. commit e81028a Author: Maxim Urschumzew <maxu@chainflip.io> Date: Mon Oct 28 15:12:47 2024 +0100 Implement `open_btc_deposit_channels`. commit 439ddd1 Author: Maxim Urschumzew <maxu@chainflip.io> Date: Mon Oct 28 14:51:22 2024 +0100 Add boilerplate for `open_btc_deposit_channels`. commit a3835f1 Author: Maxim Urschumzew <maxu@chainflip.io> Date: Mon Oct 28 10:25:51 2024 +0100 Add endpoint for marking btc tx as tainted.

codecov · 2024-10-30T10:11:20Z

Codecov Report

Attention: Patch coverage is 10.00000% with 72 lines in your changes missing coverage. Please review.

Project coverage is 71%. Comparing base (93d1677) to head (9dbb8ce).
Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
state-chain/runtime/src/lib.rs	15%	0 Missing and 34 partials ⚠️
api/lib/src/lib.rs	0%	15 Missing ⚠️
state-chain/runtime/src/runtime_apis.rs	12%	12 Missing and 3 partials ⚠️
state-chain/custom-rpc/src/lib.rs	0%	8 Missing ⚠️

Additional details and impacted files

@@          Coverage Diff           @@
##            main   #5341    +/-   ##
======================================
- Coverage     72%     71%    -0%     
======================================
  Files        497     497            
  Lines      86388   86285   -103     
  Branches   86388   86285   -103     
======================================
- Hits       61785   61558   -227     
- Misses     21822   21908    +86     
- Partials    2781    2819    +38

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

dandanlen

Looking good - I have one larger (and very annoying) suggestion and some minor ones...

dandanlen · 2024-10-30T11:00:53Z

api/bin/chainflip-broker-api/Cargo.toml

 sp-rpc = { workspace = true }
+sc-rpc = { workspace = true, default-features = true }


Duplicate 👀

FWIW I think sp-core should also have default-features=true

Unfortunately one is sP- and the other sC-... The latter was required for the sc_rpc::utils::pipe_from_stream() utility.

I am not sure about the default-features though, I copied it from the custom_rpc lib. In fact it compiles if I remove the default-features=true for sc-rpc.

Should I enable default-features for all three of them anyways?

Yes we should enable it for all three - it might not be a problem now, but this kind of thing tends to cause problems unexpectedly.

api/bin/chainflip-broker-api/src/main.rs

dandanlen · 2024-10-30T11:14:05Z

api/lib/src/lib.rs

+			.submit_signed_extrinsic(state_chain_runtime::RuntimeCall::BitcoinIngressEgress(
+				pallet_cf_ingress_egress::Call::mark_transaction_as_tainted { tx_id },
+			))
+			.await;


Can we return the result here instead of ignoring it?

The return type of that .await is (H256, (<Self as SignedExtrinsicApi>::UntilInBlockFuture, <Self as SignedExtrinsicApi>::UntilFinalizedFuture)), I am now returning the first tx_hash: H256 element.

Sorry I didn't explain very well - I meant propagating the error using ?

The return type is confusing:

The H256 is the state chain transaction hash. There is a trait implemented on the tuple that allows you to wait either until_in_block or until_finalized(). I think we should wait until_in_block.

Oh yeah and we can use submit_signed_extrinsic_with_dry_run 😅 this will avoid sending the tx if it will have no effect / won't succeed.

There's a helper trait/method for all of the above: simple_submission_with_dry_run.

Yes I see, done.

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

state-chain/runtime/src/runtime_apis.rs

state-chain/runtime/src/lib.rs

In particular the new API methods are now generic over the target chain type to keep the interface as stable as possible. However, they are still only implemented for bitcoin.

dandanlen

Some final comments then we're good.

dandanlen · 2024-10-31T13:39:21Z

state-chain/runtime/src/lib.rs

+				.filter(|channel_details| match channel_details.action {
+					ChannelAction::Swap {..} => true,
+					ChannelAction::CcmTransfer {..} => true,
+					ChannelAction::LiquidityProvision {..} => false,


I'm not sure there's a need for this filter - why ignore LiquidityProvision ?

Talked with Martin: Currently we only implement the rejection of transactions which originate from swaps. The liquidity provision transactions would have to be monitored by members of the "whitelist" which are authorized to do so. We don't have the whitelist currently, so nobody monitors these transactions.

Yes but this rpc is called cf_get_open_deposit_channels so it should probably return all of the open deposit channels for the given id.

If a broker submits this request via the broker api it will implicitly filter out an LP channels anyway. And later on when we want to be able to monitor all channels, we won't get any surprises in the behaviour of this rpc.

dandanlen · 2024-10-31T13:59:22Z

api/lib/src/lib.rs

+			.submit_signed_extrinsic(state_chain_runtime::RuntimeCall::BitcoinIngressEgress(
+				pallet_cf_ingress_egress::Call::mark_transaction_as_tainted { tx_id },
+			))
+			.await;


Sorry I didn't explain very well - I meant propagating the error using ?

The return type is confusing:

The H256 is the state chain transaction hash. There is a trait implemented on the tuple that allows you to wait either until_in_block or until_finalized(). I think we should wait until_in_block.

Oh yeah and we can use submit_signed_extrinsic_with_dry_run 😅 this will avoid sending the tx if it will have no effect / won't succeed.

There's a helper trait/method for all of the above: simple_submission_with_dry_run.

…n_with_dry_run()`.

Base automatically changed from feature/pro-1664/close-deposit-channel to main October 24, 2024 13:07

MxmUrw force-pushed the feature/PRO-1706/update-broker-api-for-broker-level-screening branch from af966d1 to a3835f1 Compare October 28, 2024 10:01

MxmUrw changed the base branch from main to feature/pro-1712/reject-tx October 28, 2024 13:55

MxmUrw changed the title ~~feat: update broker api for broker level screening~~ feat: update custom_rpc, runtime_api and broker api for broker level screening Oct 28, 2024

MxmUrw marked this pull request as ready for review October 29, 2024 14:33

MxmUrw requested a review from dandanlen as a code owner October 29, 2024 14:33

Base automatically changed from feature/pro-1712/reject-tx to main October 29, 2024 16:47

MxmUrw force-pushed the feature/PRO-1706/update-broker-api-for-broker-level-screening branch from 3166701 to e1a6006 Compare October 30, 2024 09:19

dandanlen reviewed Oct 30, 2024

View reviewed changes

MxmUrw added 2 commits October 30, 2024 16:03

Apply suggestions from review.

b2f64cb

In particular the new API methods are now generic over the target chain type to keep the interface as stable as possible. However, they are still only implemented for bitcoin.

Better name.

9dbb8ce

MxmUrw requested a review from dandanlen October 30, 2024 15:48

dandanlen reviewed Oct 31, 2024

View reviewed changes

PR suggestions: add default-features=true and use `simple_submissio…

6620ee8

…n_with_dry_run()`.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: update custom_rpc, runtime_api and broker api for broker level screening #5341

feat: update custom_rpc, runtime_api and broker api for broker level screening #5341

MxmUrw commented Oct 21, 2024 •

edited

Loading

MxmUrw commented Oct 29, 2024

codecov bot commented Oct 30, 2024 •

edited

Loading

dandanlen left a comment

dandanlen Oct 30, 2024

MxmUrw Oct 30, 2024

dandanlen Oct 31, 2024

dandanlen Oct 30, 2024

MxmUrw Oct 30, 2024

dandanlen Oct 31, 2024

MxmUrw Oct 31, 2024

dandanlen left a comment

dandanlen Oct 31, 2024

MxmUrw Oct 31, 2024 •

edited

Loading

dandanlen Oct 31, 2024

dandanlen Oct 31, 2024

		sp-rpc = { workspace = true }
		sc-rpc = { workspace = true, default-features = true }

feat: update custom_rpc, runtime_api and broker api for broker level screening #5341

Are you sure you want to change the base?

feat: update custom_rpc, runtime_api and broker api for broker level screening #5341

Conversation

MxmUrw commented Oct 21, 2024 • edited Loading

Pull Request

Checklist

Summary

Non-Breaking changes

MxmUrw commented Oct 29, 2024

codecov bot commented Oct 30, 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

dandanlen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MxmUrw Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MxmUrw commented Oct 21, 2024 •

edited

Loading

codecov bot commented Oct 30, 2024 •

edited

Loading

MxmUrw Oct 31, 2024 •

edited

Loading