feat: added a poc relayer implementation #123
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #123 +/- ##
=======================================
Coverage 93.89% 93.89%
=======================================
Files 9 9
Lines 311 311
=======================================
Hits 292 292
Misses 19 19 ☔ View full report in Codecov by Sentry. |
| // IbcEurekaTestSuite is a suite of tests that wraps TestSuite | ||
| // and can provide additional functionality | ||
| type RelayerTestSuite struct { | ||
| IbcEurekaTestSuite |
There was a problem hiding this comment.
Can I assume we will merge this into the same TestSuite eventually? Because having this kind of inheritance-like structure does not feel great.
There was a problem hiding this comment.
I want to keep them separate. We already use inheritance for IbcEurekaTestSuite.
I think inheritance is totally fine since these are just tests and it makes our lives simpler.
There was a problem hiding this comment.
Yeah, but 2 levels of inheritance is starting to get a bit crazy :) That said, maybe the fact that we would want to inherit from the IbcEurekaTestSuite just means that we should put all of that into the base level TestSuite. Why keep multiple levels if we always just need the same stuff. But we don't have to deal with that in this PR ofc
There was a problem hiding this comment.
Yeah, let's deal with it in another PR. I actually have another PR coming up which doesn't inherit from IbcEurekaTestSuite but inherits from TestSuite because it only deploys the sp1-ics07 client.
You should inherit IbcEurekaTestSuite if you need to deploy all the IbcEureka contracts. The relayer test suite needs this, so it inherits it. But other test suites might want to test other contracts/features in isolation
| /// The `ChainListener` listens for events on the Cosmos SDK chain. | ||
| pub struct ChainListener(HttpClient); | ||
|
|
||
| impl ChainListener { |
There was a problem hiding this comment.
Is it a listener? It seems to be more a of query service, no?
There was a problem hiding this comment.
Depends on how you interpret the word. I'd say it is not an event-source. But it does fetch txs, blocks, and parses events. So I feel ok calling it a listener, what do you think?
There was a problem hiding this comment.
Typically, a listener does not actively fetch information, they listen for events. So, if this was an event sink that would be fed events from the chain, it could be a listener. But this one just fetches by itself, so I don't think I would use the listener term here.
|
This is an impressive piece of work! Really well done! I will probably need to spend some time in the code more closely to see how everything fits together as this is quite a lot to review 😅 |
There was a problem hiding this comment.
It looks good! Left a couple of questions. I also think I will want review the whole structure again later when more changes are coming, but for now we are almost good to go.
I still would like both the submitter and the listener to change names as it confused me to what they really do (which is build transactions and query for events)
| provider: P, | ||
| tm_client: HttpClient, | ||
| proof_type: SupportedProofType, | ||
| sp1_private_key: Option<String>, |
There was a problem hiding this comment.
Since this is going to be an env var anyway, maybe we could avoid having to deal with sending this around and storing it in config files. Keeping private keys in config files seems like something we might want to avoid.
There was a problem hiding this comment.
The relayer should not use env variables. Sp1 should switch to api keys perhaps. I view this as an API key anyway. So this is fine
|
|
||
| let tm_listener = cosmos_sdk::ChainListener::new(tm_client.clone()); | ||
|
|
||
| let wallet = EthereumWallet::from( |
There was a problem hiding this comment.
Since we don't need to sign anything, this seems unnecessary?
There was a problem hiding this comment.
Yes this is unnecessary. But I think nothing wrong with keeping it for now, I can remove it in the subsequent PR where we add actual relaying.
Description
closes: #122
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
godoccomments.Files changedin the GitHub PR explorer.SonarCloud Reportin the comment section below once CI passes.