feat: Expose vault swaps from ingress-egress-tracker #5463

j4m1ef0rd · 2024-12-04T00:17:12Z

Pull Request

Closes: PRO-1825

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

Refactored the tracker code to use proper traits instead of exploiting the from/into impls.
Added a new data enum VaultDeposit that gets stored in the database. It is stored using the key vault_deposit:{chain}:{tx_id}.
Added a unit test for the vault deposit.
For every vault deposit the tracker will call an RPC to get the brokers registered affiliates and map them to the short id's in the vault deposit data.
New Wrapper TrackerStateChainClient to assist with mocking RPC calls.

codecov · 2024-12-05T03:17:07Z

Codecov Report

Attention: Patch coverage is 46.87500% with 17 lines in your changes missing coverage. Please review.

Project coverage is 71%. Comparing base (1a6856c) to head (85dc30d).
Report is 7 commits behind head on main.

Files with missing lines	Patch %	Lines
state-chain/chains/src/address.rs	48%	11 Missing ⚠️
...ne/src/state_chain_observer/client/base_rpc_api.rs	25%	3 Missing ⚠️
state-chain/chains/src/dot/serializable_address.rs	0%	3 Missing ⚠️

Additional details and impacted files

@@          Coverage Diff           @@
##            main   #5463    +/-   ##
======================================
- Coverage     72%     71%    -0%     
======================================
  Files        495     500     +5     
  Lines      88168   88263    +95     
  Branches   88168   88263    +95     
======================================
- Hits       63149   62889   -260     
- Misses     22450   22796   +346     
- Partials    2569    2578     +9

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

niklasnatter

looks good to me from a product perspective otherwise!

api/bin/chainflip-ingress-egress-tracker/src/witnessing/state_chain.rs

niklasnatter · 2024-12-09T17:23:20Z

api/bin/chainflip-ingress-egress-tracker/src/witnessing/state_chain.rs

+		deposit_metadata: Option<CcmDepositMetadata>,
+		deposit_details: Option<DepositDetails>,
+		broker_fee: Beneficiary<AccountId32>,
+		affiliate_fees: Affiliates<AffiliateShortId>,


would be great to have the account ids instead of the short ids here

This is turning out to be a non-trivial change. Can we leave this for a later update?

unfortunately we will need to do a separate rpc call to handle the short ids on our side, so i would really prefer to have them in redis

its fine for me to do that in a follow up pr obv, but that follow up should land before we start using it 🙂

I managed to get it working. Not the nicest, but it will do. At least we are one step closer to storing all swap data in redis. Shouldn't be too hard now to fetch the deposit channel data and get it in the db.

niklasnatter · 2024-12-09T17:32:05Z

...flip_ingress_egress_tracker__witnessing__state_chain__tests__handle_vault_deposit_calls.snap

+expression: "store.storage.get(\"vault_deposit:Ethereum:b5c8bd9430b6cc87a0e2fe110ece6bf527fa4f170a4bc8cd032f768fc5219838\").unwrap()"
+snapshot_kind: text
+---
+{"affiliate_fees":[],"amount":"0x64","boost_fee":0,"broker_fee":{"account":"5C4hrfjw9DjXZTzV3MwzrrAr9P1MJhSrvWGWqi1eSuyUpnhM","bps":0},"dca_params":null,"deposit_chain_block_height":1,"deposit_details":null,"deposit_metadata":null,"destination_address":{"Eth":[0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0]},"input_asset":{"asset":"ETH","chain":"Ethereum"},"output_asset":{"asset":"FLIP","chain":"Ethereum"},"refund_params":{"min_price":"0x0","refund_address":{"Eth":"0x541f563237a309b3a61e33bdf07a8930bdba8d99"},"retry_duration":0}}


it looks like the destination_address is formatted differently than the refund_adress here. think it would make sense to have them both exposed as string

would be cool to have some affiliates, dca params, ccm params in here to see the encoding

and the account id uses the substrate ss58 prefix instead of the cf prefix

Looks fixed now

api/bin/chainflip-ingress-egress-tracker/src/witnessing/state_chain.rs

dandanlen · 2024-12-17T16:03:46Z

api/bin/chainflip-ingress-egress-tracker/src/witnessing/state_chain.rs

+					TransactionInIdForAnyChain::Bitcoin(hash) => hex::encode(hash.as_bytes()),
+					TransactionInIdForAnyChain::Evm(hash) => hex::encode(hash.as_bytes()),
+					TransactionInIdForAnyChain::Polkadot(transaction_id) => format!(
+						"{}-{}",
+						transaction_id.block_number, transaction_id.extrinsic_index
+					),
+					TransactionInIdForAnyChain::Solana((address, id)) => format!("{address}-{id}",),
+					TransactionInIdForAnyChain::MockEthereum(_) |
+					TransactionInIdForAnyChain::None => {
+						return Err(anyhow!("Invalid transaction id: {tx_id:?}"));


We should just impl Display for TransactionInIdForAnyChain ?

dandanlen · 2024-12-17T16:16:30Z

api/bin/chainflip-ingress-egress-tracker/src/witnessing/state_chain.rs

+#[async_trait]
+trait DepositIntoWitnessInformation<C: Chain> {
+	async fn into_witness_information<StateChainClient>(
+		self,
+		height: <C as Chain>::ChainBlockNumber,
+		network: NetworkEnvironment,
+		state_chain_client: Arc<StateChainClient>,
+	) -> anyhow::Result<WitnessInformation>
+	where
+		StateChainClient: StorageApi + ChainApi + TrackerApi + 'static + Send + Sync;
+}


This looks like it can be simplified - no need for async or sc client? (Or result??)

Suggested change

#[async_trait]

trait DepositIntoWitnessInformation<C: Chain> {

async fn into_witness_information<StateChainClient>(

self,

height: <C as Chain>::ChainBlockNumber,

network: NetworkEnvironment,

state_chain_client: Arc<StateChainClient>,

) -> anyhow::Result<WitnessInformation>

where

StateChainClient: StorageApi + ChainApi + TrackerApi + 'static + Send + Sync;

}

trait DepositIntoWitnessInformation<C: Chain> {

async fn into_witness_information<StateChainClient>(

self,

height: <C as Chain>::ChainBlockNumber,

network: NetworkEnvironment,

) -> WitnessInformation;

}

(Also, not sure we really need a macro)

Actually - this is pretty confusing - we have two macros for implementing the same trait 😅

dandanlen · 2024-12-17T16:25:29Z

api/bin/chainflip-ingress-egress-tracker/src/witnessing/state_chain.rs

+impl_vault_deposit_into_witness_information!(Polkadot);
+impl_vault_deposit_into_witness_information!(Arbitrum);
+
+impl BroadcastIntoWitnessInformation for BroadcastDetails<Ethereum> {


... but no macro for this?

dandanlen · 2024-12-17T16:27:57Z

api/bin/chainflip-ingress-egress-tracker/src/witnessing/state_chain.rs

+	}
+}
+
+async fn save_deposit_witnesses<


I feel like we could simplify a lot if we query for a map of affiliate IDs in this method, and then pass that through, rather than threading the sc client (and attendant type constraints and async-ness) down through the whole call stack.

dandanlen · 2024-12-17T16:28:32Z

api/bin/chainflip-ingress-egress-tracker/src/witnessing/state_chain.rs

+	if let Some(broadcast_details) =
+		get_broadcast_id::<I, StateChainClient>(state_chain_client, &tx_out_id)
+			.await
+			.map(|broadcast_id| BroadcastDetails::<I> { broadcast_id, tx_out_id, tx_ref })


(...A bit like this approach)

j4m1ef0rd requested a review from kylezs December 4, 2024 00:17

j4m1ef0rd self-assigned this Dec 4, 2024

Base automatically changed from feat/vault-swap-boost to main December 4, 2024 07:33

j4m1ef0rd force-pushed the feat/vault-swaps-in-tracker2 branch from 7c874b8 to 753fb2d Compare December 5, 2024 03:03

j4m1ef0rd force-pushed the feat/vault-swaps-in-tracker2 branch 2 times, most recently from b6f28c0 to bc331f7 Compare December 5, 2024 05:45

j4m1ef0rd added the CLI / API label Dec 9, 2024

j4m1ef0rd requested a review from niklasnatter December 9, 2024 00:08

j4m1ef0rd marked this pull request as ready for review December 9, 2024 00:09

j4m1ef0rd requested a review from a team as a code owner December 9, 2024 00:09

j4m1ef0rd requested review from zoheb391 and removed request for a team December 9, 2024 00:09

niklasnatter reviewed Dec 9, 2024

View reviewed changes

j4m1ef0rd force-pushed the feat/vault-swaps-in-tracker2 branch 2 times, most recently from 9a868fe to 0443269 Compare December 13, 2024 05:46

kylezs self-assigned this Dec 16, 2024

kylezs marked this pull request as draft December 16, 2024 04:16

kylezs changed the title ~~Expose vault swaps from ingress-egress-tracker~~ feat: Expose vault swaps from ingress-egress-tracker Dec 16, 2024

kylezs marked this pull request as ready for review December 16, 2024 05:09

j4m1ef0rd and others added 7 commits December 16, 2024 16:11

feat: tracker refactor and support for vault swaps

d14c5a6

refactor: map affiliates & address PR comments

aa602e3

refactor: clean up save_deposit_witnesses

801df39

refactor: use map to access directly

74bec41

refactor: remove tracker struct

5ade4d4

refactor: pull out common vault witness calls into macro

cd6c426

refactor: pull out deposit witness common into macro

536a354

kylezs force-pushed the feat/vault-swaps-in-tracker2 branch from 5a92f35 to 536a354 Compare December 16, 2024 05:11

dandanlen added 2 commits December 16, 2024 11:28

fix: downgrade lockfile version

9d86420

chore: add polkadot address

c3da775

dandanlen force-pushed the feat/vault-swaps-in-tracker2 branch from bed8ed2 to c3da775 Compare December 16, 2024 10:29

dandanlen reviewed Dec 17, 2024

View reviewed changes

WIP (I'll fix this then structure the commits and push again)

85dc30d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Expose vault swaps from ingress-egress-tracker #5463

feat: Expose vault swaps from ingress-egress-tracker #5463

j4m1ef0rd commented Dec 4, 2024 •

edited by kylezs

Loading

codecov bot commented Dec 5, 2024 •

edited

Loading

niklasnatter left a comment

niklasnatter Dec 9, 2024

j4m1ef0rd Dec 12, 2024

niklasnatter Dec 12, 2024

niklasnatter Dec 12, 2024

j4m1ef0rd Dec 13, 2024

niklasnatter Dec 9, 2024

niklasnatter Dec 9, 2024

acdibble Dec 9, 2024

kylezs Dec 16, 2024

dandanlen Dec 17, 2024

dandanlen Dec 17, 2024

dandanlen Dec 17, 2024

dandanlen Dec 17, 2024

dandanlen Dec 17, 2024

dandanlen Dec 17, 2024

dandanlen Dec 17, 2024

feat: Expose vault swaps from ingress-egress-tracker #5463

Are you sure you want to change the base?

feat: Expose vault swaps from ingress-egress-tracker #5463

Conversation

j4m1ef0rd commented Dec 4, 2024 • edited by kylezs Loading

Pull Request

Checklist

Summary

codecov bot commented Dec 5, 2024 • edited Loading

Codecov Report

niklasnatter 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

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

j4m1ef0rd commented Dec 4, 2024 •

edited by kylezs

Loading

codecov bot commented Dec 5, 2024 •

edited

Loading