Skip to content
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

ledger-tool: Deprecate bank-hash command #1710

Merged
merged 2 commits into from
Jun 13, 2024

Conversation

steviez
Copy link

@steviez steviez commented Jun 12, 2024

Problem

ledger-tool has a somewhat cluttered interface. The bank-hash command
offers little extra functionality compared to what the verify command
does, and could instead be an optional flag for the verify command.

Summary of Changes

This change leaves the bank-hash command present, but prints an error
message and tells the user to instead use verify --print-bank-hash. The
command will be removed altogether in the future.

steviez added 2 commits June 12, 2024 14:34
ledger-tool in general has a lot of commands that can be tough to
navigate. This command in general is pretty minimal, and could seemingly
be an optional flag to verify.

For now, leave the command present but print an error message. This
command will completely removed in the future
@steviez
Copy link
Author

steviez commented Jun 12, 2024

I'm open to general thoughts on this one. My thinking is that the functionality offered by the command is minimal, and doesn't warrant a command. I have similar thoughts for shred-version command, which I will do after this PR if we like the direction of this PR. On that vein, suppose someone wanted to know the bank hash AND shred-version; they would currently need to run the tool twice whereas having flags in verify command allows someone to do multiple things in one run.

@steviez steviez marked this pull request as ready for review June 12, 2024 22:12
@steviez steviez requested a review from CriesofCarrots June 12, 2024 22:12
Copy link

@CriesofCarrots CriesofCarrots left a comment

Choose a reason for hiding this comment

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

Seems reasonable to me

@steviez steviez merged commit 6d8fa7a into anza-xyz:master Jun 13, 2024
40 checks passed
@steviez steviez deleted the lt_dep_bank_hash branch June 13, 2024 17:13
samkim-crypto pushed a commit to samkim-crypto/agave that referenced this pull request Jul 31, 2024
ledger-tool has a somewhat cluttered interface. The bank-hash command
offers little extra functionality compared to what the verify command
does, and could instead be an optional flag for the verify command.

This change leaves the bank-hash command present, but prints an error
message and tells the user to instead use verify --print-bank-hash. The
command will be removed altogether in the future.
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.

2 participants