-
Notifications
You must be signed in to change notification settings - Fork 41
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
Feat: Implement cardano-db-v2
command in client CLI (list
and show
)
#2266
base: main
Are you sure you want to change the base?
Feat: Implement cardano-db-v2
command in client CLI (list
and show
)
#2266
Conversation
8b17d0a
to
9fee61d
Compare
9fee61d
to
b9c7088
Compare
b9c7088
to
48c0b88
Compare
cardano-db-v2
command in client CLIcardano-db-v2
command in client CLI (list
and show
)
48c0b88
to
91ebcaf
Compare
91ebcaf
to
d77b3f1
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.
LGTM 👍
.github/workflows/test-client.yml
Outdated
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.
Maybe the namings should be a bit more consistent between Cardano Database v1 and v2
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.
Done ✅
format!("{}", item.beacon.immutable_file_number).cell(), | ||
item.hash.cell(), | ||
item.merkle_root.cell(), | ||
item.total_db_size_uncompressed.cell(), |
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.
Can you display this size in a human readable format?
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.
Should I use an external crate such as humansize?
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.
Maybe displaying the size in GB
is sufficient
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.
Done ✅
vec!["Merkle root".cell(), cardano_db_message.merkle_root.cell()], | ||
vec![ | ||
"Database size".cell(), | ||
format!("{}", &cardano_db_message.total_db_size_uncompressed).cell(), |
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.
Same comment here
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.
Done ✅
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
assert!(row.is_some()); | ||
|
||
let row = digest_location_row(456, DigestLocationDiscriminants::Aggregator, &locations); | ||
assert!(row.is_some()); |
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 wonder if a test that show the row rendering could be interesting
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 code was refactored after your comment were posted.
Since CellStruct doesn't implement Eq
or Display
, I managed to test it by recreating a TableStruct
. Is that ok for you?
- add 'cdbv2' alias for 'cardano-db-v2' - implement 'cardano-db-v2 list' command
…ril Client multi-platform test
…ril Client multi-platform test
…nd v2 in Mithril Client multi-platform test
c867e69
to
609d8f2
Compare
@@ -42,6 +42,12 @@ impl CardanoDbUtils { | |||
res = future => res, | |||
} | |||
} | |||
|
|||
pub fn format_bytes_to_gigabytes(bytes: u64) -> 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.
The unit you are using is incorrect:
- 1 gibibyte (GiB) = 1024**3 bytes
- 1 gigabyte (GB) = 1000**3 bytes
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 see. Should I update it to use GiB
instead of GB
and leave the calculation (1024**3 bytes) unchanged?
That way, it will match the unit used in the explorer.
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, you can use GiB
as in the explorer 👍
609d8f2
to
b7f3557
Compare
* mithril-client-cli from `0.10.8` to `0.10.9` * mithril-common from `0.4.110` to `0.4.111` * mithril-end-to-end from `0.4.65` to `0.4.66`
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 🔥
Content
This PR introduces a new
unstable
command (cardano-db-v2
with aliascdbv2
) for the Mithril client CLI, allowing users to list and show details of Incremental Cardano DB snapshots.Example for
snapshot list
command:Terminal output:
Example for
snapshot show
command:Terminal output:
Additionally, the E2E test and the Mithril Client multi-platform test manual workflow have been updated to verify both
cardano-db-v2
commands (show
andlist
).Pre-submit checklist
Issue(s)
Relates to #2246