Skip to content

shim: query CLI #34

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

Merged
merged 5 commits into from
Nov 22, 2023
Merged

shim: query CLI #34

merged 5 commits into from
Nov 22, 2023

Conversation

pepyakin
Copy link
Contributor

This changeset introduces a command allowing querying the sugondat from the CLI. At this time it only adds submit functionality: submit a blob to a given namespace.

As such this change is more about laying framework.

Builds on top of #32.

@@ -100,6 +100,11 @@ pub mod serve {
pub mod query {
//! CLI definition for the `query` subcommand.

// TODO: I envision several subcommands here. For example:
// - query block <block_hash/number> — returns the information about a block and header.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these are query parameters for the DA layer block / number, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep! generally shim doesn't know anything about rollup blocks. Only DA blocks and blobs.

Copy link
Contributor

@rphmeier rphmeier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good on top of #32

let signed = self
.subxt
.tx()
.create_signed(&extrinsic, &from, Default::default())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this creating a default key?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://docs.rs/subxt/latest/subxt/tx/struct.TxClient.html#method.create_signed

I don't quite understand what's happening here and what the type of the created Signer is.

Copy link
Contributor Author

@pepyakin pepyakin Nov 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

create_signed + the next thing is actually inlining this function

https://github.com/thrumdev/sugondat/blob/16bf54cdc99dfdbafae64b5fb18168310807f56e/sugondat-da-adapter/src/service.rs#L214-L220

I did this in an attempt to make the error message a bit less confusing. I have no idea what other_data (specified by Default::default) is and why it is used. The signer key is passed by from.

Copy link
Contributor

@rphmeier rphmeier Nov 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, makes sense. I missed the from parameter somehow.

The other_data would be SignedExtra stuff, I guess, which might be desired for mortality in particular.

@rphmeier rphmeier mentioned this pull request Nov 22, 2023
@pepyakin pepyakin merged commit 8b7228a into main Nov 22, 2023
@pepyakin pepyakin deleted the pep-shim-rpc-cli branch November 22, 2023 10:29
/// two, the byte vector must be prefixed with `0x`.
///
/// The integer is interpreted as little-endian.
fn read_namespace(namespace: &str) -> anyhow::Result<sugondat_nmt::Namespace> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

another possible implementation would be:

fn read_namespace(namespace: &str) -> anyhow::Result<sugondat_nmt::Namespace> {

    if let Some(hex) = namespace.strip_prefix("0x") {
        let namespace = hex::decode(hex)?;
        let namespace: [u8; 4] = namespace.try_into().map_err(|e: Vec<u8>| {
            anyhow::anyhow!("namespace must be 4 bytes long, but was {}", e.len())
        })?;
        Ok(sugondat_nmt::Namespace::from_raw_bytes(namespace))
    }

    let namespace_id = namespace
        .parse::<u32>()
        .with_context(|| format!("cannot parse namespace id '{}'", namespace))?;
    Ok(sugondat_nmt::Namespace::with_namespace_id(namespace_id))
}

or cleaner but technically more computation required because the from_str_radix would perform an useless translation to u32

fn read_namespace(mut namespace: &str) -> anyhow::Result<sugondat_nmt::Namespace> {

    let namespace_id = match namespace.strip_prefix("0x") {
        Some(hex) => u32::from_str_radix(hex, 16).with_context(|| format!("cannot parse namespace id '{}'", namespace))?,
        None => namespace.parse::<u32>().with_context(|| format!("cannot parse namespace id '{}'", namespace))?
    };

    Ok(sugondat_nmt::Namespace::with_namespace_id(namespace_id))
}

(didn't test the error handling)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hah, this is a funny one.

I specifically went for the implementation that you see here to disallow passing 0x0x0011, because those functions are not equivalent: one removes the occurance once and the other removes all occurances. However, I mixed them up and did the opposite what I inteded.

That said, I don't think this matters much and it's OK either way. Feel free to file a PR with the fix. I actually prefer the first option: for one, it's not unlikely that we change the size of the namespace #12 . Note that you it seems you are missing a return though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants