Feat: New vault swap encoding RPC & bouncer test #5384

j4m1ef0rd · 2024-11-05T04:47:13Z

Pull Request

Closes: PRO-1754 & PRO-1752

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

New vault swap encoding RPC to the statechain.
Refactored the vault swap encoding RPC on the broker API to just be a pass through.
New bouncer test that uses the new RPC to get the encoded data and deposit address, then constructs the Btc transaction and broadcasts it and waits for the swap to go through.

TODO

Return private channel address
Update documentation
Add test to the concurrent tests and possibly add more test swaps.
When using the SC RPC's, errors appear with no information, just RpcError: -32603: {error}.

dandanlen · 2024-11-05T20:32:55Z

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

@@ -86,20 +84,6 @@ pub trait Rpc {
 		asset: Asset,
 		destination_address: AddressString,
 	) -> RpcResult<WithdrawFeesDetail>;
-
-	#[method(name = "request_swap_parameter_encoding", aliases = ["broker_requestSwapParameterEncoding"])]
-	async fn request_swap_parameter_encoding(


I think we want to keep this in the broker api, and pass it through to the custom rpc.

dandanlen · 2024-11-05T21:31:05Z

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

+#[derive(Clone, Serialize, Deserialize)]
+pub enum EncodedVaultSwapParamsBytes {


I think it would be nice to use internally tagged enum variants here.

Suggested change

#[derive(Clone, Serialize, Deserialize)]

pub enum EncodedVaultSwapParamsBytes {

#[derive(Clone, Serialize, Deserialize)]

#[serde(tag = "chain")]

pub enum EncodedVaultSwapParamsBytes {

dandanlen · 2024-11-05T21:39:25Z

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

@@ -68,6 +69,33 @@ use std::{
 pub mod monitoring;
 pub mod order_fills;

+#[derive(Clone, Serialize, Deserialize)]
+pub struct VaultSwapDetailsHumanreadable {
+	pub deposit_address: ForeignChainAddressHumanreadable,


We won't need to return an address for every chain - it's only for bitcoin where it depends on the broker_id and current epoch. For other chains there is a vault contract, which is static. We can still return this static address I suppose but calling it a deposit_address is not always accurate.

I would suggest putting this inside the enum's bitcoin variant.

dandanlen · 2024-11-05T21:46:55Z

state-chain/runtime/src/runtime_apis.rs

+
+#[derive(PartialEq, Eq, Clone, Encode, Decode, TypeInfo, Serialize, Deserialize)]
+pub enum EncodedVaultSwapParams {
+	Bitcoin { nulldata_utxo: Vec<u8> },


Let's just use Bytes everywhere?

Bytes does not have TypeInfo, which the runtime apis need for some reason.

Oh yeah... helpful.

I had a dig, we can do this:

Bitcoin { #[serde(with = "sp_core::bytes")] nulldata_utxo: Vec<u8> }, }

dandanlen · 2024-11-05T21:48:32Z

state-chain/runtime/src/runtime_apis.rs

 type VanityName = Vec<u8>;

+#[derive(PartialEq, Eq, Clone, Encode, Decode, TypeInfo, Serialize, Deserialize)]
+pub struct VaultSwapDetails {
+	pub deposit_address: ForeignChainAddress,


We can do the conversion in the runtime so we don't need separate types here vs in the rpc layer.

Take a look at chainflip::ChainAddressConverter.

I was unable to find a easy way to output the address as hex without using 2 structs or changing EncodedAddress.

dandanlen · 2024-11-05T22:00:06Z

state-chain/runtime/src/lib.rs

+						output_address: to_encoded_address(
+							destination_address,
+							Environment::network_environment,
+						),


Suggested change

output_address: to_encoded_address(

destination_address,

Environment::network_environment,

),

output_address: ChainAddressConverter::to_encoded_address(

destination_address,

),

dandanlen · 2024-11-05T22:02:13Z

state-chain/runtime/src/lib.rs

+								.unwrap_or(1)
+								.try_into()
+								.map_err(|_| {
+									Into::<DispatchErrorWithMessage>::into(DispatchError::from(


These error conversions are quite repetitive - maybe we just need to write the correct From implementation for DispatchErrorWithMessage?

At the very least we could write a struct method to make this more readable. Something like this might do the trick:

impl DispatchErrorWithMessage { pub fn from_pallet_error(e: impl Into<DispatchError>) { Self::from(e.into()) } }

dandanlen · 2024-11-05T22:07:12Z

state-chain/runtime/src/lib.rs

+							chunk_interval: dca_parameters
+								.as_ref()
+								.map(|params| params.chunk_interval)
+								.unwrap_or(2)


Can we avoid using a magic number here? Isn't there a constant defined in the swapping pallet?

dandanlen · 2024-11-05T22:11:40Z

state-chain/runtime/src/lib.rs

+					let btc_key = pallet_cf_threshold_signature::Pallet::<Runtime, BitcoinInstance>::keys(
+						pallet_cf_threshold_signature::Pallet::<Runtime, BitcoinInstance>::current_key_epoch().unwrap()).unwrap();


If you import the KeyProvider trait you can use BitcoinThresholdSigner::active_epoch_key() for this.

This code will get removed when i hook it up to the private channels, so i will leave it for now.

dandanlen · 2024-11-05T22:14:08Z

state-chain/runtime/src/lib.rs

+					let deposit_address = ForeignChainAddress::Btc(
+						cf_chains::btc::deposit_address::DepositAddress::new(btc_key.current, 0)
+							.script_pubkey(),
+					);


We can convert to an EncodedAddress here.

If I output as EncodedAddress it will not be a hex string.

codecov · 2024-11-08T04:23:22Z

Codecov Report

Attention: Patch coverage is 20.08547% with 187 lines in your changes missing coverage. Please review.

Project coverage is 71%. Comparing base (daf9e73) to head (6a6d84e).
Report is 7 commits behind head on main.

Files with missing lines	Patch %	Lines
state-chain/runtime/src/lib.rs	14%	0 Missing and 78 partials ⚠️
state-chain/custom-rpc/src/lib.rs	0%	33 Missing ⚠️
state-chain/chains/src/address.rs	22%	30 Missing and 2 partials ⚠️
state-chain/runtime/src/runtime_apis.rs	45%	17 Missing and 1 partial ⚠️
api/lib/src/lib.rs	0%	11 Missing ⚠️
state-chain/pallets/cf-swapping/src/lib.rs	10%	9 Missing ⚠️
api/lib/src/lp.rs	0%	5 Missing ⚠️
...ate-chain/traits/src/mocks/swap_limits_provider.rs	0%	1 Missing ⚠️

Additional details and impacted files

@@          Coverage Diff           @@
##            main   #5384    +/-   ##
======================================
- Coverage     72%     71%    -0%     
======================================
  Files        486     486            
  Lines      85973   85977     +4     
  Branches   85973   85977     +4     
======================================
- Hits       61526   61383   -143     
- Misses     21586   21662    +76     
- Partials    2861    2932    +71

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

bouncer/tests/btc_vault_swap.ts

api/lib/src/lib.rs

dandanlen · 2024-11-11T10:24:11Z

LGTM now, but I made some changes/refactors:

The error handling could be simplified further by writing the impl for T: Into<DispatchError> instead of DispatchError.
There was no need to change how the BrokerApi trait was implemented.
Similarly, there was no need to add any methods to the BaseRpcApi, we can just pass through straight to the node via the raw client.
I remove the Option from the runtime api as I mentioned above. The runtime api methods are scale encoded, so they don't benefit from being able to 'leave out' the values as we can with a json api.
I deduplicated some code from the bitcoin address return type. For example, there's no need for a whole separate type. Also, no need for a ForeignChainAddress since it is always a bitcoin address.
I also added the lookup of the private broker id - this was easy to add, i don't see any need to add it in another PR.

kylezs · 2024-11-11T10:54:30Z

state-chain/runtime/src/lib.rs

-							.script_pubkey(),
-					);
+					let EpochKey { key, .. } = BitcoinThresholdSigner::active_epoch_key()
+						.expect("We should always have a for the current epoch.");


We should always have a key for the current epoch.

even before vault setup on a localnet?

j4m1ef0rd · 2024-11-12T03:35:50Z

I also added the lookup of the private broker id - this was easy to add, i don't see any need to add it in another PR.

Main reason was that it breaks the new vault swap bouncer test. I have another PR with this all working.

j4m1ef0rd added 2 commits November 5, 2024 15:23

feat: new vault swap encoding rpc

6401c48

test: Btc vault swap bouncer test

8d9ff2a

j4m1ef0rd added State Chain Bouncer labels Nov 5, 2024

j4m1ef0rd requested a review from msgmaxim November 5, 2024 04:47

j4m1ef0rd self-assigned this Nov 5, 2024

j4m1ef0rd requested review from dandanlen and martin-chainflip as code owners November 5, 2024 04:47

dandanlen reviewed Nov 5, 2024

View reviewed changes

j4m1ef0rd added 8 commits November 6, 2024 11:54

Merge branch 'main' into feat/vault-swap-rpc

29ce9ad

refactor: address some PR comments

58dbf09

chore: rename bouncer evm vault swap file

5e2870e

feat: RPC in broker api and address some PR comments

124e5e1

Merge branch 'main' into feat/vault-swap-rpc

cd57633

refactor: go back to better error types on encoded address

9b304ab

Merge branch 'main' into feat/vault-swap-rpc

c00685e

chore: change prewitness origin logic get test to pass

e2327ca

kylezs reviewed Nov 8, 2024

View reviewed changes

bouncer/tests/btc_vault_swap.ts Show resolved Hide resolved

bouncer/tests/btc_vault_swap.ts Outdated Show resolved Hide resolved

kylezs reviewed Nov 8, 2024

View reviewed changes

api/lib/src/lib.rs Outdated Show resolved Hide resolved

j4m1ef0rd and others added 7 commits November 11, 2024 10:37

chore: Address PR comments for bouncer test

bf8460c

chore: use BlockNumber instead of u32 for retry duration

3399d75

feat: simplified error handling

e16a110

fix: pass-through base_rpc

3422e8a

feat: use direct access to node rpcs

afc539c

chore: remove options

c8f64dc

fix: simplify bitcoin address return type

611ce46

kylezs approved these changes Nov 11, 2024

View reviewed changes

fix: comments + clippy

6a6d84e

dandanlen added this pull request to the merge queue Nov 11, 2024

Merged via the queue into main with commit 023261c Nov 11, 2024
49 checks passed

dandanlen deleted the feat/vault-swap-rpc branch November 11, 2024 13:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: New vault swap encoding RPC & bouncer test #5384

Feat: New vault swap encoding RPC & bouncer test #5384

j4m1ef0rd commented Nov 5, 2024 •

edited

Loading

dandanlen Nov 5, 2024

dandanlen Nov 5, 2024

dandanlen Nov 5, 2024

dandanlen Nov 5, 2024

j4m1ef0rd Nov 6, 2024

dandanlen Nov 6, 2024

dandanlen Nov 5, 2024

j4m1ef0rd Nov 7, 2024

dandanlen Nov 5, 2024

dandanlen Nov 5, 2024

dandanlen Nov 5, 2024

dandanlen Nov 5, 2024

j4m1ef0rd Nov 6, 2024

dandanlen Nov 5, 2024

j4m1ef0rd Nov 8, 2024

codecov bot commented Nov 8, 2024 •

edited

Loading

dandanlen commented Nov 11, 2024

kylezs Nov 11, 2024

j4m1ef0rd Nov 12, 2024

j4m1ef0rd commented Nov 12, 2024

		#[derive(Clone, Serialize, Deserialize)]
		pub enum EncodedVaultSwapParamsBytes {

		let btc_key = pallet_cf_threshold_signature::Pallet::<Runtime, BitcoinInstance>::keys(
		pallet_cf_threshold_signature::Pallet::<Runtime, BitcoinInstance>::current_key_epoch().unwrap()).unwrap();

Feat: New vault swap encoding RPC & bouncer test #5384

Feat: New vault swap encoding RPC & bouncer test #5384

Conversation

j4m1ef0rd commented Nov 5, 2024 • edited Loading

Pull Request

Checklist

Summary

TODO

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

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

codecov bot commented Nov 8, 2024 • edited Loading

Codecov Report

dandanlen commented Nov 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

j4m1ef0rd commented Nov 12, 2024

j4m1ef0rd commented Nov 5, 2024 •

edited

Loading

codecov bot commented Nov 8, 2024 •

edited

Loading