-
Notifications
You must be signed in to change notification settings - Fork 10
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
shim: blob query #167
shim: blob query #167
Conversation
Current dependencies on/for this PR:
This stack of pull requests is managed by Graphite. |
sugondat-shim/src/cli.rs
Outdated
BlockRef::Number(n) => write!(f, "{}", n), | ||
BlockRef::Hash(h) => write!(f, "0x{}", hex::encode(&h[..])), | ||
} | ||
fn decode_hash(input: &str) -> Result<Option<[u8; 32]>, 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.
Nit: Would appreciate not mixing new features and refactorings
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.
I am not graphite-pilled enough to keep things that small. at the time, it would've required stacking on top of only some of the changes within the file, stashing the other ones, and then popping the stash to build on top of the refactored decoder
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.
Fair enough. I suggested that because for me it seemed like a simple operation.
sugondat-shim/src/cmd/query/blob.rs
Outdated
.block_hash(n) | ||
.await? | ||
.ok_or_else(|| anyhow::anyhow!("No block with number {}", n))? | ||
.0, |
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.
goddammit. I was trying to guard the API from precisely this thing and failed!
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.
Specifically the .0
thing?
I did notice there's a comment in the sugondat_rpc
file: "NOTE: we specifically avoid prolifiration of subxt types around the codebase. To that end, we avoid returning H256 and instead return [u8; 32] directly."
And yet, pub async fn block_hash(&self, height: u64) -> anyhow::Result<Option<H256>>
returns the H256. Seems like an easy fix, though.
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's precisely what I meant. If you look at blame, my name will pop on both of those lines.
sugondat-shim/src/cmd/query/blob.rs
Outdated
} else { | ||
println!( | ||
" Blob #{}, Namespace {}, {} bytes", | ||
i + 1, |
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.
Not sure if it's a good idea to have the descrepancy between the cli input and here. In the CLI it's the zero-based and here it's 1-based.
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.
The extrinsic index is zero-based, whereas the blob index is 1-based. if we need consistency between two (different) indexes, I would prefer they both be 0-based rather than 1-based.
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.
Opened #170 & we can discuss there
f79eecd
to
93059ee
Compare
589760b
to
110b39a
Compare
93059ee
to
7cae1e7
Compare
110b39a
to
4b642aa
Compare
4b642aa
to
1feca1e
Compare
No description provided.