-
Notifications
You must be signed in to change notification settings - Fork 11.2k
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
Add bcs
field to Checkpoint
#20340
Add bcs
field to Checkpoint
#20340
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Skipped Deployments
|
crates/sui-graphql-e2e-tests/tests/stable/call/checkpoint_connection_pagination.exp
Outdated
Show resolved
Hide resolved
crates/sui-graphql-e2e-tests/tests/stable/consistency/epochs/checkpoints.exp
Outdated
Show resolved
Hide resolved
635d64a
to
11fd0e8
Compare
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.
This implementation is changing things it doesn't need to, which is why I think you're getting in trouble with the pagination of regular checkpoints, it would be better to implement it as follows:
- Implement a DataLoader for raw checkpoint information (keyed by sequence number).
- Implement the
bcs
field as aload_one
for that data loader. - Delete everything else because you don't need it.
That makes total sense, thanks so much. @wlmyng was hinting at that yesterday but I did not get it at the time. Much nicer to write! |
cf3d557
to
6ca6f21
Compare
checkpointSummaryBcs
field to Checkpoint
bcs
field to Checkpoint
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.
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.
Accepting modulo the nits! Thanks @stefan-mysten
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.
lgtm as well, module Ashok's comments
Description
This PR adds the
bcs
for Checkpoint to enable GraphQL Client to correctly handleCheckpointSummary
data.Test plan
Added the field to two existing tests and updated the snapshots. A bit uncertain why some things have changed, so would need some help to figure out if it's correct or not. @wlmyng
Release notes
Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.
For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.
Checkpoint
now has abcs
field that represents the Base64 encoded BCS serialization of theCheckpointSummary
data.