feat: Provide ProtocolStreamBuilder to simplify decoding #53

kayibal · 2024-11-10T09:48:49Z

No description provided.

kayibal · 2024-11-29T09:32:29Z

Cargo.toml

+tycho-core = { git = "https://github.com/propeller-heads/tycho-indexer.git", package = "tycho-core", rev = "f8d765554194ddd222e4c6f07811e8c99700615a" }
+tycho-client = { git = "https://github.com/propeller-heads/tycho-indexer.git", package = "tycho-client", rev = "f8d765554194ddd222e4c6f07811e8c99700615a" }
+tycho-ethereum = { git = "https://github.com/propeller-heads/tycho-indexer.git", package = "tycho-ethereum", rev = "f8d765554194ddd222e4c6f07811e8c99700615a" }


I have to changes these

Is there a PR we need to review on tycho-indexer that correlates to this PR then?

No it's already merged

kayibal · 2024-11-29T09:33:34Z

examples/explorer/main.rs

+        let mut protocol_stream = ProtocolStreamBuilder::new(&tycho_url, Chain::Ethereum)
+            .exchange::<UniswapV2State>(
+                "uniswap_v2",
+                ComponentFilter::with_tvl_range(cli.tvl_threshold, cli.tvl_threshold),
+            )
+            .exchange::<UniswapV3State>(
+                "uniswap_v3",
+                ComponentFilter::with_tvl_range(cli.tvl_threshold, cli.tvl_threshold),
+            )
+            .auth_key(Some(tycho_api_key.clone()))
+            .set_tokens(all_tokens)
+            .build()
+            .await
+            .expect("Failed building protocol stream");


This is how you subscribe with tycho-simulation to protocol state updates now.

kayibal · 2024-11-29T09:38:12Z

src/evm/protocol/vm/utils.rs

+    sync_get_code(connection_string.as_str(), addr)
+}
+
+fn sync_get_code(connection_string: &str, addr: H160) -> Result<Bytecode, SimulationError> {


I need to add a comment here:

The Provider futures are not Sync, so this would break the try_from_with_block trait requirements. I fixed this by moving the not sync part to a separate thread. It's not great ideally we should upgrade to alloy and hope their futures are Sync...

Just to confirm: this is only used if tycho does not provide the code right? Which ideally shouldn't happen..

can you add this in a TODO in the code please? I feel that here it will get lost easily

No I think this is mainly used to get stateless contracts.

@dianacarvalho1 will fix this before merging.

louise-poole

A couple questions/suggestions... but for the most part it looks great 👍

src/protocol/models.rs

louise-poole · 2024-11-29T09:55:56Z

src/protocol/state.rs

@@ -55,6 +51,8 @@ use crate::{
        models::GetAmountOutResult,
    },
 };
+use ethers::types::U256;
+use tycho_core::dto::ProtocolStateDelta;


FMI: why are you moving all the imports around like this? We've been trying to keep them divided with:

external imports

other tycho repos imports

internal imports

There has been a lot less duplicate import lines since we've done this... by duplicate imports I mean avoiding situations where you import different things from the same file in multiple lines:

use crate::protocol::errors::SimulationError; other imports... use crate::protocol::errors::TransitionError;

This is also solved by us removing all new lines between the imports I guess. But that means we have giant import blocks for some files which isn't too nice. I think we need to choose a convention and stick to it.

I just cargo +nightly format I had a lot of terrible merge errors because of this... How do you do it?

src/protocol/stream_decoder.rs

louise-poole · 2024-11-29T10:21:41Z

src/protocol/stream_decoder.rs

+    pub new_pairs: HashMap<String, ProtocolComponent>,
+    /// The pairs that were removed in this block
+    pub removed_pairs: HashMap<String, ProtocolComponent>,


Question: why do we need to track new/removed pools when we have a Hashmap of all the current pools anyway?

louise-poole · 2024-11-29T10:30:31Z

src/evm/protocol/vm/utils.rs

+    sync_get_code(connection_string.as_str(), addr)
+}
+
+fn sync_get_code(connection_string: &str, addr: H160) -> Result<Bytecode, SimulationError> {


Just to confirm: this is only used if tycho does not provide the code right? Which ideally shouldn't happen..

louise-poole · 2024-11-29T10:42:18Z

src/protocol/decoder.rs

    use crate::{
-        models::ERC20Token,
        evm::protocol::uniswap_v2::state::UniswapV2State,
-        protocol::{
-            decoder::{StreamDecodeError, TychoStreamDecoder},
-        },
+        models::ERC20Token,
+        protocol::decoder::{StreamDecodeError, TychoStreamDecoder},
    };
    use ethers::types::U256;
    use rstest::*;


Why not use use super::*; here and skip re-importing many of these imports already used in this file?

louise-poole · 2024-11-29T10:43:07Z

Cargo.toml

+tycho-core = { git = "https://github.com/propeller-heads/tycho-indexer.git", package = "tycho-core", rev = "f8d765554194ddd222e4c6f07811e8c99700615a" }
+tycho-client = { git = "https://github.com/propeller-heads/tycho-indexer.git", package = "tycho-client", rev = "f8d765554194ddd222e4c6f07811e8c99700615a" }
+tycho-ethereum = { git = "https://github.com/propeller-heads/tycho-indexer.git", package = "tycho-ethereum", rev = "f8d765554194ddd222e4c6f07811e8c99700615a" }


Is there a PR we need to review on tycho-indexer that correlates to this PR then?

louise-poole · 2024-11-29T10:44:29Z

src/protocol/decoder.rs

I'm not sure this should belong in the protocol directory per se... it feels more higher level than the protocol specifics housed here. Maybe we can have a stream directory under src?

louise-poole · 2024-11-29T10:44:47Z

src/protocol/stream.rs

Same for this.

dianacarvalho1

Thank you @kayibal 🙏🏼 Only have small questions/requests.
However.. you removed the vm support entirely? 😢 can you bring it back please?

dianacarvalho1 · 2024-11-29T10:36:06Z

src/evm/protocol/uniswap_v2/state.rs

+use ethers::types::U256;
+use std::{any::Any, collections::HashMap};
+use tycho_core::{dto::ProtocolStateDelta, Bytes};


@louise-poole is going to be mad about this import order 👀
I thought we were doing:

std imports

external imports

tycho

local project

(I didn't see her review before posting mine 😂 )

VM is supported I just haven't added it to the example. Any protocol state that implement the TryFromWithBlock trait is automatically supported no need to change anything on the code.

dianacarvalho1 · 2024-11-29T10:40:32Z

src/evm/protocol/vm/utils.rs

+    sync_get_code(connection_string.as_str(), addr)
+}
+
+fn sync_get_code(connection_string: &str, addr: H160) -> Result<Bytecode, SimulationError> {


can you add this in a TODO in the code please? I feel that here it will get lost easily

src/protocol/decoder.rs

dianacarvalho1 · 2024-11-29T10:47:00Z

examples/explorer/ui.rs

            let entry = self
                .items
                .iter()
-                .find_position(|e| e.component.address == *address);
+                .find_position(|e| e.component.address == eth_address);


I would rename this to ethereum_address. eth_address seems it's the ETH address

I think ethereum_address and eth_address have the same meaning in my head. For ETH address I would call it the native_address since that's what it's been throughout our defibot code - so in my opinion this is fine

dianacarvalho1 · 2024-11-29T10:47:32Z

src/protocol/decoder.rs

Can we rename this file to tycho_stream_decoder?

It's not limited to Tycho I hope...

louise-poole · 2024-11-29T11:02:10Z

src/protocol/stream.rs

+    pub fn set_tokens(self, tokens: HashMap<Bytes, ERC20Token>) -> Self {
+        self.decoder.set_tokens(tokens);
+        self
+    }


Can we document this. When do we need tokens set? Is it only when we stream vm protocols or is it needed for native too?

It's always needed to correctly create ProtocolComponents structs.

dianacarvalho1 · 2024-12-12T12:00:15Z

src/evm/protocol/vm/assets/BalancerV2SwapAdapter.evm.runtime

huumm this was already done in master 😕

Adjust the decoder for the changed interfaces. Also adjust the interfaces to accept a reference to a hashmap instead of passing hashmaps or vectors by value.

The decoder has currently no trait and is evm specific. The stream module is pretty general but depends atm on the decoder since there is no trait for it.

dianacarvalho1

Thank you so much @kayibal 🙏🏼 this was a hard baby!!

tamaralipows

That's great - a lot cleaner. Thank you 🙏

tamaralipows · 2024-12-13T15:29:12Z

src/evm/decoder.rs

+        guard.tokens = tokens;
+    }
+
+    // Method to register a decoder


Pretty useless comment

Suggested change

// Method to register a decoder

Yeah I need to still do the docs for this one

## [0.60.0](0.59.3...0.60.0) (2024-12-13) ### Features * Provide ProtocolStreamBuilder to simplify decoding ([#53](#53)) ([bc82193](bc82193))

propellerci · 2024-12-13T15:34:29Z

This PR is included in version 0.60.0 🎉

kayibal force-pushed the ah/simplify-decoding branch 4 times, most recently from 537f337 to 97edb21 Compare November 29, 2024 08:43

kayibal changed the title ~~WIP: simplify decoding~~ feat: Provide ProtocolStreamBuilder to simplify decoding Nov 29, 2024

kayibal commented Nov 29, 2024

View reviewed changes

louise-poole approved these changes Nov 29, 2024

View reviewed changes

dianacarvalho1 requested changes Nov 29, 2024

View reviewed changes

louise-poole reviewed Nov 29, 2024

View reviewed changes

kayibal force-pushed the ah/simplify-decoding branch 2 times, most recently from eb7d340 to 4f89479 Compare December 12, 2024 11:33

dianacarvalho1 reviewed Dec 12, 2024

View reviewed changes

src/evm/protocol/vm/assets/BalancerV2SwapAdapter.evm.runtime Outdated

Copy link

Collaborator

dianacarvalho1 Dec 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

huumm this was already done in master 😕

kayibal force-pushed the ah/simplify-decoding branch 3 times, most recently from 049d505 to 36db1dd Compare December 12, 2024 20:39

kayibal added 13 commits December 13, 2024 13:16

feat: Simplify instantiating a protocol stream

d51ed1c

feat: Unify stream building into a single builder.

e3b5bce

feat: Simpler module name

115cfcb

feat: Get example working with new stream.

e622081

feat: Move things around and add tests

618de9e

feat: Adjust decoder for changed interfaces.

96c6665

Adjust the decoder for the changed interfaces. Also adjust the interfaces to accept a reference to a hashmap instead of passing hashmaps or vectors by value.

fix: Fix new file paths

00f51e5

fmt

2a5dbe6

clippy

76003c7

WIP: fix test

5210e81

Post rebase fixes

bc72cd0

Add VM storage updates to decoder

a7fb190

Add client side component filter predicates

37ba7da

kayibal added 3 commits December 13, 2024 13:18

Misc fixes

cbe4e3c

Post rebase fixes

84f7095

Activate curve

da5d737

kayibal force-pushed the ah/simplify-decoding branch from c80fa1a to d3bc09d Compare December 13, 2024 12:19

Move decoder an stream modules under evm.

27969d8

The decoder has currently no trait and is evm specific. The stream module is pretty general but depends atm on the decoder since there is no trait for it.

kayibal force-pushed the ah/simplify-decoding branch 3 times, most recently from e0e5416 to 87895bc Compare December 13, 2024 14:48

Make set_token async

b832495

kayibal force-pushed the ah/simplify-decoding branch from 87895bc to b832495 Compare December 13, 2024 15:04

kayibal enabled auto-merge (squash) December 13, 2024 15:20

dianacarvalho1 approved these changes Dec 13, 2024

View reviewed changes

kayibal merged commit bc82193 into main Dec 13, 2024
3 checks passed

kayibal deleted the ah/simplify-decoding branch December 13, 2024 15:31

tamaralipows approved these changes Dec 13, 2024

View reviewed changes

propellerci bot pushed a commit that referenced this pull request Dec 13, 2024

chore(release): 0.60.0 [skip ci]

8ea6072

## [0.60.0](0.59.3...0.60.0) (2024-12-13) ### Features * Provide ProtocolStreamBuilder to simplify decoding ([#53](#53)) ([bc82193](bc82193))

propellerci bot added the true label Dec 13, 2024

feat: Provide ProtocolStreamBuilder to simplify decoding #53

feat: Provide ProtocolStreamBuilder to simplify decoding #53

Conversation

kayibal commented Nov 10, 2024

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

louise-poole 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

dianacarvalho1 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

dianacarvalho1 left a comment

Choose a reason for hiding this comment

tamaralipows left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

propellerci bot commented Dec 13, 2024