[GraphQL Client] Fix digests #59
Conversation
stefan-mysten
force-pushed
the
gql_fix_digests
branch
from
c77e63c to
17876b2
Compare
crates/sui-graphql-client/src/lib.rs
Outdated
| @@ -643,7 +646,7 @@ impl Client { | |||
| /// provided, it will use the last known checkpoint id. | |||
| pub async fn checkpoint( | |||
| &self, | |||
| digest: Option<Digest>, | |||
| digest: Option<CheckpointContentsDigest>, | |||
There was a problem hiding this comment.
this probably should be CheckpointDigest and not Contents given the return type here
There was a problem hiding this comment.
The thing that confuses me is that, if I want to get the total transaction blocks in a checkpoint by digest after using this checkpoint function to get the checkpoint data, I have to pass in the CheckpointContentsDigest because that's what's content_digest type is.
pub content_digest: CheckpointContentsDigest,
pub previous_digest: Option<CheckpointDigest>,
So I would have to convert CheckpointContentsDigest to CheckpointDigest.
Not sure I fully follow why we need to have this separation.
There was a problem hiding this comment.
You should take a look at the checkpoint types in the sdk types crate to familiarize yourself for what the differences are. The CheckpointDigest is the digest of a CheckpointSummary while the CheckpointContentsDigest is the digest of the "contents" or the CheckpointContents type which contains the list of txn digests and user signatures
There was a problem hiding this comment.
That's what I gathered as well, but I think then we're missing a digest field in the CheckpointSummary type.
There was a problem hiding this comment.
Actually, I am confused.
| let digest = chckp.unwrap().unwrap().content_digest; | ||
| let chckp_digest = CheckpointDigest::from_bytes(digest.inner()).unwrap(); | ||
| let total_transaction_blocks = client | ||
| .total_transaction_blocks_by_digest(digest.into()) | ||
| .await; | ||
| assert!(total_transaction_blocks.is_ok()); | ||
| assert!(total_transaction_blocks.unwrap().is_some_and(|tx| tx > 0)); | ||
| .total_transaction_blocks_by_digest(chckp_digest) |
There was a problem hiding this comment.
This is...odd...and seems inaccurate. total_transaction_blocks_by_digest should take a CheckpointDigest and not use the contents digest.
There was a problem hiding this comment.
Thanks for catching this. CheckpointSummary has a previous_digest and content_digest field, but no digest field which I would expect would return the checkpoint's digest.
There was a problem hiding this comment.
sui-rust-sdk/crates/sui-sdk-types/src/types/checkpoint.rs
Lines 72 to 73 in f7921a1
There was a problem hiding this comment.
In GraphQL schema, the digest of Checkpoint is
"""
A 32-byte hash that uniquely identifies the checkpoint contents, encoded in Base58. This
hash can be used to verify checkpoint contents by checking signatures against the committee,
Hashing contents to match digest, and checking that the previous checkpoint digest matches.
"""
digest: String!
There was a problem hiding this comment.
that's because it needs to be calculated, its a hash over the type, its not a field on the actual type. Although graphql likely exposes the digest as a field
There was a problem hiding this comment.
Got it, thank you!
Yes, indeed, in GraphQL it's exposed as a field - following where it's being computed, I think this is the code.
https://github.com/stefan-mysten/sui/blob/2cf6f86853604a1ba8c34cc2dc3b54dba47e402f/crates/sui-types/src/messages_checkpoint.rs?plain=1#L194-L201
|
A proper fix for the |
This PR fixes digests to use the respective types. One thing that is confusing is the difference between
CheckpointDigestandCheckpointContentsDigest.