feat: add websocket eth subscription to deposit tracker #4081

msgmaxim · 2023-10-09T02:34:26Z

Pull Request

Partially addresses: PRO-868

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

I ran into a few problems integrating other chains, so I thought it'd be better to merge eth tracking first so that the product team can start integrating this if they want and/or to get early feedback.

As discussed with @kylezs, I decided to copy just what's needed from the engine's witnessing rather than reusing the entire witnessing module. I did try the latter approach, but found that it would require a lot of changes to make it work for both binaries without unwanted side effects (just to name a few issues: (1) the engine requires a database, while the deposit tracker does not; (2) deposits are post-witnessed in the engine, but we want them pre-witnessed by the deposit tracker; (3) we don't want chain tracking in the deposit tracker etc). Copying individual witnesser components results in some duplication, but I don't think it is a problem (it is just initialisation code rather than logic), and I think we can do some refactoring to help with this too.

Changes:

Added subscribe_eth subscription endpoint to the rpc client, which forwards all witnessed events substrate-codec-encoded (From our discussion I got the impression that this works for you @niklasnatter? If not, we can discuss other options.)
For now all settings are hardcoded (to match how it is currently done for BTC). We should probably make it easier to configure in the future, but I decided it was out of scope for this PR. Annoyngly, our witnessing components reqiure a signing capable ETH client even though we don't need signing for deposit tracking. As a temporary hack, I create a temporary file with a dummy key, so I can pass it to the witnessing code and make it happy. (I figured it'd be easier than asking the product team to provide such dummy file some other way, and I think we can revisit this when we add cofiguration options.)
All eth witnessing is done in a separate module (we should probably move BTC stuff in a separate module too). Here for the most part we initialise witnesser components the same way we do for pre-witnessing in the engine. Rather than being submitted as extrinsics, events are communicated via a broadcast channel to WS subscribers. This includes: deposit witnessing of USDC, FLIP, ETH and vault and chainflip-gateway contract witnessing.

FYI @niklasnatter

(Tracking of other chains will come shortly, but I might need to do some refactoring first)

codecov · 2023-10-09T02:43:55Z

Codecov Report

Merging #4081 (b527050) into main (27fd688) will increase coverage by 0%.
Report is 13 commits behind head on main.
The diff coverage is n/a.

@@          Coverage Diff          @@
##            main   #4081   +/-   ##
=====================================
  Coverage     71%     71%           
=====================================
  Files        376     377    +1     
  Lines      59875   59965   +90     
  Branches   59875   59965   +90     
=====================================
+ Hits       42623   42718   +95     
+ Misses     15013   15008    -5     
  Partials    2239    2239

Files	Coverage Δ
engine/src/witness/eth.rs	`0% <ø> (ø)`

... and 31 files with indirect coverage changes

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

kylezs

Nice 👍 I think it's a good idea to get at least ETH working and in first as there's also an infrastructure component, this will need to be setup as part of the web stack. As well as the web component itself, to actually use the tracker.

kylezs · 2023-10-09T06:11:04Z

api/bin/chainflip-btc-deposit-tracker/Cargo.toml

@@ -15,3 +15,17 @@ serde = "1.0.183"
 tokio = "1.29.1"
 tracing = "0.1.34"
 tracing-subscriber = { version = "0.3.3", features = ["env-filter"] }
+tempfile = "3.8"


we should change the name of these files and the binary name now to ingress-egress-tracker

Changed to chainflip-ingress-egress-tracker. I think I have updated all the scripts that used the old name, but maybe @ahasna could double check?

kylezs · 2023-10-09T11:30:58Z

api/bin/chainflip-btc-deposit-tracker/src/eth.rs

+	let vault_address = state_chain_client
+		.storage_value::<pallet_cf_environment::EthereumVaultAddress<state_chain_runtime::Runtime>>(
+			state_chain_client.latest_finalized_hash(),
+		)
+		.await
+		.context("Failed to get Vault contract address from SC")?;
+
+	let state_chain_gateway_address = state_chain_client
+        .storage_value::<pallet_cf_environment::EthereumStateChainGatewayAddress<state_chain_runtime::Runtime>>(
+            state_chain_client.latest_finalized_hash(),
+        )
+        .await
+        .context("Failed to get StateChainGateway address from SC")?;
+
+	let address_checker_address = state_chain_client
+		.storage_value::<pallet_cf_environment::EthereumAddressCheckerAddress<state_chain_runtime::Runtime>>(
+			state_chain_client.latest_finalized_hash(),
+		)
+		.await
+		.expect("State Chain client connection failed");


I think we do this before starting any of the witnessing processes. As this is effetively configuration that if it doesn't exist, should start this process at all.

api/bin/chainflip-btc-deposit-tracker/src/main.rs

api/bin/chainflip-btc-deposit-tracker/src/eth.rs

api/bin/chainflip-btc-deposit-tracker/src/main.rs

api/bin/chainflip-btc-deposit-tracker/src/eth.rs

api/bin/chainflip-btc-deposit-tracker/src/main.rs

ci/docker/dev/chainflip-btc-deposit-tracker.Dockerfile

kylezs · 2023-10-10T07:36:26Z

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

+	supported_erc20_tokens: HashMap<H160, cf_primitives::Asset>,
+}
+
+async fn get_env_parameters(state_chain_client: &StateChainClient<()>) -> EnvironmentParameters {


we could use this in the main witnesser too

The engine's code for this is a bit scattered. Do you want all of these to be read in engine's main? That's where the eth client currently created, for example.

kylezs · 2023-10-10T07:41:55Z

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

+	settings: DepositTrackerSettings,
+	witness_sender: tokio::sync::broadcast::Sender<state_chain_runtime::RuntimeCall>,
+) {
+	tokio::spawn(async {


is there a reason we do this spawn? we also use a task::spawn in the btc deposit tracker section. Would be good to make these consistent. We can do so by wrapping the whole thing in the task_scope and then calling each in their own scopes - as I think you may be suggesting in this TODO

Yeah, the todo is for that. Should be an easy change, but would move a lot of code around, so figured it'd be cleaner to do it separately.

kylezs

If you want to push the comments I just left into issues/PRs so we can unblock any of the product/infra work, I'm ok with that

msgmaxim · 2023-10-11T01:08:55Z

If you want to push the comments I just left into issues/PRs so we can unblock any of the product/infra work, I'm ok with that

Yeah, I prefer making the changes in the next PR. Tested on localnet before merging. Actually looks like broadcasting returns error when there aren't any subscribers, so removed unwrap to better handle this case. Also, I renamed subscription endpoints to more generic subscribe_witnessing so that we don't have to change them when we add other chains.

feat: add websocket eth subscription to deposit tracker

b413520

msgmaxim requested review from kylezs and martin-chainflip October 9, 2023 02:34

kylezs reviewed Oct 9, 2023

View reviewed changes

api/bin/chainflip-btc-deposit-tracker/src/main.rs Outdated Show resolved Hide resolved

api/bin/chainflip-btc-deposit-tracker/src/main.rs Outdated Show resolved Hide resolved

msgmaxim added 8 commits October 10, 2023 14:50

chore: address minor review comments

7d379d9

chore: rename event_sender -> witness_sender

0a66424

feat: change chainflip-btc-tracker to chainflip-ingress-egress-tracker

1e9494d

refactor: move eth under (new) witnessing module

7e44530

refactor: move shared logic and initialisation out of eth module

139bde4

feat: configuration via env variables

cbadcd2

chore: clippy fix

e467b8f

chore: rename docker files

8a706ba

kylezs reviewed Oct 10, 2023

View reviewed changes

kylezs approved these changes Oct 10, 2023

View reviewed changes

fix: ignore send error if no subscribers

3a8b2ac

msgmaxim enabled auto-merge (squash) October 11, 2023 01:09

msgmaxim disabled auto-merge October 11, 2023 01:10

chore: rename subscription endpoints

30701b3

msgmaxim enabled auto-merge (squash) October 11, 2023 01:15

msgmaxim disabled auto-merge October 11, 2023 01:34

fix: ensure connections are always correctly closed

b527050

msgmaxim enabled auto-merge (squash) October 11, 2023 01:36

msgmaxim merged commit eb669d2 into main Oct 11, 2023
44 checks passed

msgmaxim deleted the feat/eth-deposit-tracker branch October 11, 2023 02:05

msgmaxim mentioned this pull request Oct 11, 2023

Feat: ensure correct process termination in ingress/egress tracker #4101

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add websocket eth subscription to deposit tracker #4081

feat: add websocket eth subscription to deposit tracker #4081

msgmaxim commented Oct 9, 2023 •

edited

Loading

codecov bot commented Oct 9, 2023 •

edited

Loading

kylezs left a comment

kylezs Oct 9, 2023

msgmaxim Oct 10, 2023

kylezs Oct 9, 2023

kylezs Oct 10, 2023

msgmaxim Oct 11, 2023

kylezs Oct 10, 2023

msgmaxim Oct 10, 2023

kylezs left a comment

msgmaxim commented Oct 11, 2023 •

edited

Loading

feat: add websocket eth subscription to deposit tracker #4081

feat: add websocket eth subscription to deposit tracker #4081

Conversation

msgmaxim commented Oct 9, 2023 • edited Loading

Pull Request

Checklist

Summary

codecov bot commented Oct 9, 2023 • edited Loading

Codecov Report

kylezs left a comment

Choose a reason for hiding this comment

kylezs Oct 9, 2023

Choose a reason for hiding this comment

msgmaxim Oct 10, 2023

Choose a reason for hiding this comment

kylezs Oct 9, 2023

Choose a reason for hiding this comment

kylezs Oct 10, 2023

Choose a reason for hiding this comment

msgmaxim Oct 11, 2023

Choose a reason for hiding this comment

kylezs Oct 10, 2023

Choose a reason for hiding this comment

msgmaxim Oct 10, 2023

Choose a reason for hiding this comment

kylezs left a comment

Choose a reason for hiding this comment

msgmaxim commented Oct 11, 2023 • edited Loading

msgmaxim commented Oct 9, 2023 •

edited

Loading

codecov bot commented Oct 9, 2023 •

edited

Loading

msgmaxim commented Oct 11, 2023 •

edited

Loading