chore: remove google-datastore1 & ethabi #630
Conversation
There was a problem hiding this comment.
Pull request overview
This PR removes two unused dependencies: google-datastore1 (since Datastore is no longer used, only Secret Manager is needed) and ethabi (replacing it with alloy's ABI encoding capabilities that are already available in the project).
Key Changes:
- Migrated ABI encoding in
indexer_sol.rsfromethabito alloy's tuple-basedabi_encode()method - Removed
google-datastore1dependency and relatedKeyable/KeyKindtraits fromgcp/mod.rs - Cleaned up Cargo.toml and Cargo.lock files to remove both dependencies
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| chain-signatures/node/src/indexer_sol.rs | Migrated from ethabi's Token-based encoding to alloy's tuple-based abi_encode() for generating request IDs |
| chain-signatures/node/src/gcp/mod.rs | Removed google-datastore1 import and unused Keyable/KeyKind traits |
| chain-signatures/node/Cargo.toml | Removed google-datastore1 and ethabi dependencies |
| Cargo.toml | Removed ethabi workspace dependency |
| Cargo.lock | Updated lock file to reflect removed dependencies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ChaoticTempest
force-pushed
the
phuong/chore/reduce-deps
branch
from
49dce54 to
aae960c
Compare
ChaoticTempest
force-pushed
the
phuong/chore/reduce-deps
branch
from
aae960c to
2b352ba
Compare
jakmeier
left a comment
jakmeier
left a comment
There was a problem hiding this comment.
Makes sense.
Although, judging by the Cargo.lock, we still pull in all the same dependencies indirectly. So the benefit of removing these direct dependencies is somewhat limited. But still useful IMO.
Oh and it seems you missed to update indexer_hydration.rs. Currently it doesn't compile.
|
ahh, yeah ethers is pulling in ethabi. I'll get rid of it in favor of purely alloy. The google-datastore1 isn't in our dependency tree anymore though |
| let mut hasher = Keccak256::new(); | ||
| hasher.update(&encoded); | ||
| hasher.finalize().into() | ||
| // Encode the event data in ABI format using alloy |
There was a problem hiding this comment.
would you mind checking if the old and new implementation generates the same request id? This is important as this need to be the same as the request id that our demo is checking for. Sometimes small changes in encoding changes the value of the request id.
Thanks!!!!
A unit test for this function is a better approach in the longer run.
There was a problem hiding this comment.
good call, the underlying alloy abi_encode actually appends an extra header so it definitely changed all the encodings. Let me see what I can do about that and I'll add a unit test
| Token::String(self.dest.clone()), | ||
| Token::String(self.params.clone()), | ||
| ]); | ||
| // Encode the event data in ABI format using alloy |
ChaoticTempest
force-pushed
the
phuong/chore/reduce-deps
branch
from
d670e16 to
49b9a09
Compare
volovyks
left a comment
volovyks
left a comment
There was a problem hiding this comment.
Great reduction in dependencies! All the updates look reasonable to me.
Ethereum is not covered well with integration tests. Have you tested it locally?
Removes google-datastore1 in favor of just using google-secretmanager1 since we no longer use datastore for anything. We can also just directly use alloy instead of ethabi