Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support for the Hydration blockchain as a new chain indexer, following the established patterns from Ethereum and Solana indexers. The implementation includes bidirectional transaction support and creates a common indexer module to reduce code duplication across chain implementations.
Key Changes
- Adds
indexer_hydrationmodule with Merkle-proof verified event indexing from Hydration blockchain - Creates
indexer_commonmodule to consolidate shared indexer logic across Solana, Ethereum, and Hydration - Extends the
Chainenum and related infrastructure to support Hydration chain
Reviewed changes
Copilot reviewed 17 out of 19 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
chain-signatures/node/src/indexer_hydration/mod.rs |
New Hydration indexer implementation with event processing and Merkle proof verification |
chain-signatures/node/src/indexer_common.rs |
New shared indexer functionality extracted from Solana indexer |
chain-signatures/node/src/indexer_sol.rs |
Refactored to use common indexer functions |
chain-signatures/node/src/indexer_eth/mod.rs |
Refactored to use common backlog recovery |
chain-signatures/node/src/rpc.rs |
Added Hydration client and publish support |
chain-signatures/node/src/cli.rs |
Added Hydration CLI arguments |
chain-signatures/primitives/src/lib.rs |
Extended Chain enum with Hydration |
chain-signatures/crypto/src/kdf.rs |
Added Hydration epsilon derivation |
integration-tests/src/*.rs |
Updated integration test configuration |
chain-signatures/node/Cargo.toml |
Added Substrate/Polkadot dependencies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 20 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 23 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
volovyks
left a comment
There was a problem hiding this comment.
Great work! I have not found any major issues, let's move forward with this.
| Chain::Ethereum => "0x1", | ||
| Chain::Solana => "0x800001f5", | ||
| Chain::Bitcoin => "bip122:000000000019d6689c085ae165831e93", | ||
| Chain::Hydration => "polkadot:2034", |
There was a problem hiding this comment.
2034 is not a CAIP-2 identifier, it's a ParaId that identifies it inside of Polkadot network.
CAIP 2 is using the genesis hash. OS it should be polkadot:afdc188f45c71dacbaa0b62e16a91f72.
There was a problem hiding this comment.
this is matching the id that galacticcouncil/hydration-node#1275 sets in the tests.
There was a problem hiding this comment.
Then we need to change it there @esaminu and fix here
| fn main() { | ||
| let args: Vec<String> = env::args().collect(); | ||
|
|
||
| let (hydration_pair, hydration_phrase, _seed) = sr25519::Pair::generate_with_phrase(None); |
There was a problem hiding this comment.
The whole /infra folder is intended to be removed (#560). Let's added changes there. @auto-mausx please, approve that PR if we are ready to move forward with it.
| Chain::NEAR => return None, | ||
| Chain::Ethereum => ("CHECKPOINT_INTERVAL_ETHEREUM", 20), | ||
| Chain::Solana => ("CHECKPOINT_INTERVAL_SOLANA", 120), | ||
| Chain::Hydration => ("CHECKPOINT_INTERVAL_HYDRATION", 120), |
There was a problem hiding this comment.
@ChaoticTempest remind me please, why Ethereum has smaller backup interval compared to Solana and now Hydration?
There was a problem hiding this comment.
I think it's because the Ethereum block time is slower. It's better if checkpoints are never too old.
Maybe for Hydration it should be larger than for Solana?
There was a problem hiding this comment.
If Ethereum block time is slower, why do we need to back it up more often?
| // Call the existing derive_epsilon_sol function with the correct parameters | ||
| // to match the TypeScript implementation | ||
| let epsilon = derive_epsilon_sol(self.key_version, &self.sender.to_string(), &self.path); | ||
| let epsilon = derive_epsilon_sol(self.key_version, &self.sender_string(), &self.path); |
There was a problem hiding this comment.
I would say it should be sender(), not sender_string(). We should not specify type in the function name.
|
|
||
| /// Fetch and *verify* the SCALE‑encoded `System::Events` bytes at a given block. | ||
| /// | ||
| /// - Uses `state_get_read_proof` via `LegacyRpcMethods`. |
There was a problem hiding this comment.
this is for fetching proof, but I haven't been able to test it locally.
There was a problem hiding this comment.
I'm saying that this API is marked as legacy (outdated), maybe there is new one that we should use.
| if let Err(err) = sign_tx.send(Sign::Request(sign_request)).await { | ||
| // TODO: handle error to ensure 100% success rate | ||
| let chain = sign_event.source_chain(); | ||
| tracing::error!(?err, chain = %chain, "Failed to send {} sign request into queue", chain.as_str()); | ||
| } else { |
There was a problem hiding this comment.
Hm, so if this fails, we have a full channel buffer. A burst of incoming requests could easily trigger this. So anyone could try and abuser this to prevent a signature from getting through.
But the good news is, at this point we already have the request in the backlog. All we need is to ensure we start new attempts on backlog requests if no success is seen for a time. (cc @ChaoticTempest )
I know this is just moved from other files in this PR. But can we maybe link this to an issue or something to make it less likely this is just buried and forgotten?
| let target_chain = match target_chain { | ||
| Ok(chain) => chain, | ||
| Err(_) => Chain::Ethereum, | ||
| }; |
There was a problem hiding this comment.
We send to ETH if we don't know where it should go? Why?
There was a problem hiding this comment.
That's because now all the bidirectional tx's send actual transaction on Ethereum. This will need to change in the future.
There was a problem hiding this comment.
It would be safer to throw an error. I guess we are specifying target_chain=Ethereum anyway.
| Chain::NEAR => return None, | ||
| Chain::Ethereum => ("CHECKPOINT_INTERVAL_ETHEREUM", 20), | ||
| Chain::Solana => ("CHECKPOINT_INTERVAL_SOLANA", 120), | ||
| Chain::Hydration => ("CHECKPOINT_INTERVAL_HYDRATION", 120), |
There was a problem hiding this comment.
I think it's because the Ethereum block time is slower. It's better if checkpoints are never too old.
Maybe for Hydration it should be larger than for Solana?
6fe77d6 to
22f9f94
Compare
* bidirectional: hydration indexer --------- Co-authored-by: Xiangyi Zheng <xyzheng@uhicago.edu>
This works locally with https://github.com/galacticcouncil/hydration-node/pull/1189/files.
https://github.com/galacticcouncil/hydration-node/pull/1189/files has been closed, and hydration team is pushing forward with https://github.com/galacticcouncil/hydration-node/pull/1235/files#.
We can review this PR first, and I'll push new PR that aims to dynamically work with hydration runtime changes, i.e. https://github.com/galacticcouncil/hydration-node/pull/1235/files#.