feat!(sidecar): add start check callback with rollkit example #1118
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
Reecepbcups
changed the title
| ChainID: "gm", | ||
| Images: []ibc.DockerImage{ | ||
| { | ||
| Repository: "ghcr.io/gjermundgaraba/gm", |
There was a problem hiding this comment.
Is this just the standard https://github.com/rollkit/gm or are there changes made to work with this?
Ref https://github.com/rollchains/tiablob/blob/main/interchaintest/setup_celestia.go as well vs using a combined instance. I should probably get this moved here and help clean up some setup
There was a problem hiding this comment.
Just standard, but there is no updated docker builds (and the main branch is outdated) so I just took the tutorial-local-da branch and built a docker image of that
| @@ -256,6 +256,7 @@ type SidecarConfig struct { | |||
| StartCmd []string | |||
| Env []string | |||
| PreStart bool | |||
| StartCheck func(int) error | |||
| NoHostMount: false, | ||
| SkipGenTx: false, | ||
| PreGenesis: nil, | ||
| ModifyGenesis: func(config ibc.ChainConfig, bytes []byte) ([]byte, error) { |
There was a problem hiding this comment.
Ahh I see now. Ideally we would abstract this away and have a RollKit section which does this but does not have the user be informed of all the setup complexities. Everything here looks reasonable imo
great work!
I need to think on it for how we can handle. Maybe a IsRollkitChain: true (or if we can somehow figure this out without user interaction, i.e. something within the filesystem or part of the main binary cobra)
then this would all run after modify genesis with a celestia-da
There was a problem hiding this comment.
Yeah, it would be great to automagically sort this somehow without all the manual stuff for sure :)
Reecepbcups
changed the title
| StartCmd: []string{"/bin/bash", "/opt/entrypoint.sh"}, | ||
| Env: nil, // Here we could set CELESTIA_NAMESPACE if needed | ||
| PreStart: true, | ||
| StartCheck: func(index int) error { |
There was a problem hiding this comment.
note to self: Maybe to be more inline with #1123, we rename this to PostStart() on the sidecar process. Just so it more closely matches that PRs API conventions and also is more versatile vs just a standard check.
Does not seem the index here is used? Could put the CosmosChain here or something
There was a problem hiding this comment.
That naming makes sense.
Yeah, the idea was originally to send in the SidecarProcess but that gets to a cyclical dependency so ended up with an index and then in the end didn't use it.
In the code below I just do Sidecars[0], but I guess the idea was to access the right one. But I suppose I would always know the order they are added (being the same as the SidecarConfigs slice order). So could send in the chain like you said instead.
|
Hey, we have still not decided on a priority level for this PR. It is something we want to get in, but have to juggle with other priorities right now as well. Review / impl time is still unknown |
I don't know what the best way to deal with dependencies between sidecars and chains, but since I used Celestia as a sidecar for the Rollkit example it needed some time to wait for the sidecar to start up before starting the chain itself.
Putting this here for discussion.