-
Notifications
You must be signed in to change notification settings - Fork 10
shim/rollkit: go-da via gRPC #260
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Current dependencies on/for this PR: This stack of pull requests is managed by Graphite. |
a725cf3
to
eb78206
Compare
The CI is flaky. |
} | ||
|
||
#[derive(Subcommand, Debug)] | ||
pub enum Dock { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just noting: the biggest implication of this change, it seems, is that you can no longer run the shim for all docks at the same time. I am not sure why that would be useful & don't think this fundamentally impacts the product. I'm sure we could add it again later if needed, anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that was my reasoning as well.
/// big-endian integer. To distinguish between the two, the byte vector must be prefixed | ||
/// with `0x`. | ||
#[clap(long, short, env = ENV_IKURA_NAMESPACE)] | ||
pub namespace: Option<String>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mentioned in the PR description that rollkit doesn't submit blobs with a namespace. Also, when this is None
we get a CLI warning.
Bu the SubmitBlob
gRPC contains a namespace
.
What I am understanding so far is that:
- With any rollkit chain today, the namespace is omitted in the request and therefore must be configured in the shim.
- Rollkit could be altered in the future to support sending namespaces without requiring any changes to the shim
correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kind of.
I am not sure what is the reason of this interface and what is the ultimate game plan: maybe the namespace being phased out? Or maybe it got a hit during migration to go-da. Either way, right now it doesn't seem that Rollkit takes the namespace ID.
I just went to check to celestia-da (implementation of go-da that embeds the whole celestia-node) and it seems that they are also taking a mandatory namespace parameter.
This changeset migrates the rollkit demo to go-da, the more modern version of the DA interface for Rollkit. One of the most important changes is that rollkit doesn't embed the adapters anymore. Instead, it communicates with the adapter through gRPC protocol. Instead of yet another adapter, this implements gRPC dock directly, which kind of challenges the current architecture. Notable changes: 1. demo/rollkit was regenerated from the tutorial once again. That turned out to be easier than manually porting changes. I ported init-local.sh though. 2. removed adapter/sugondat since it is not longer needed 3. since it looks like we are going away from the docker workflow, I did not port the Dockerfile to the new code. I left the docker-compose.yml though, just removed the gm service. 4. the JSON-RPC was pushed to the sovereign dock. Now `serve` subcommand will launch only the specified dock instead of all of them. Each dock has its own subcommand. `ikura-shim serve rollkit` is now used for running rollkit. 5. now dock subcommands get their own options. E.g. rollkit doc now can accept the default namespace in case requests from rollkit without namespace (they do atm). There are some things that were left for follow ups: 1. #258 1. #259
eb78206
to
bc18025
Compare
&self, | ||
supplied_ns: Option<pbda::Namespace>, | ||
) -> Result<ikura_nmt::Namespace, Status> { | ||
Ok(match supplied_ns { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid wrapping everything into an Ok(...)
another approach could probably be:
match supplied_ns {
Some(pbda::Namespace {
value: raw_namespace_bytes,
}) => {
let raw_namespace_bytes = raw_namespace_bytes.as_slice();
if raw_namespace_bytes.len() != 16 {
return Err(Status::invalid_argument("namespace must be 16 bytes long"));
}
let mut namespace = [0u8; 16];
namespace.copy_from_slice(raw_namespace_bytes);
Ok(ikura_nmt::Namespace::from_raw_bytes(namespace))
}
None => self.namespace
.and_then(|namespace| Ok(namespace.clone()))
.ok_or(Status::invalid_argument("namespace must be provided"))
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks terser and like a good suggestion - @gabriele-0201 this is a good opportunity to practice on Graphite. Instead of pausing this PR and updating it, create a new PR based on this stack which applies that change as a refactor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh and yeah namespace.clone()
is not necessary.
the version that was introduced in #260 (997046d) was using the latest version of go-da (0.4.0). However, the latest version of rollkit on which the gm demo was based off, was using 0.2.0. That was causing occasional problems. So I went ahead and downgraded it. Now, it seems to workfine (with exception of #259). Closes #258.
the version that was introduced in #260 (997046d) was using the latest version of go-da (0.4.0) gRPC service decl. However, the latest version of rollkit on which the gm demo was based off, was using 0.2.0. That was causing occasional problems. So I went ahead and downgraded it. Now, it seems to workfine (with exception of #259). Closes #258.
the version that was introduced in #260 (997046d) was using the latest version of go-da (0.4.0) gRPC service decl. However, the latest version of rollkit on which the gm demo was based off, was using 0.2.0. That was causing occasional problems. So I went ahead and downgraded it. Now, it seems to workfine (with exception of #259). Closes #258.
This changeset migrates the rollkit demo to go-da, the more modern version
of the DA interface for Rollkit.
One of the most important changes is that rollkit doesn't embed the adapters anymore.
Instead, it communicates with the adapter through gRPC protocol. Instead of yet another
adapter, this implements gRPC dock directly, which kind of challenges the current
architecture.
Notable changes:
easier than manually porting changes. I ported init-local.sh though.
the Dockerfile to the new code. I left the docker-compose.yml though, just removed
the gm service.
serve
subcommand will launchonly the specified dock instead of all of them. Each dock has its own subcommand.
ikura-shim serve rollkit
is now used for running rollkit.namespace in case requests from rollkit without namespace (they do atm).
There are some things that were left for follow ups:
Closes #106