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

rpc: add ChainHash message with both bytes and string representations #1087

Merged
merged 1 commit into from
Aug 15, 2024

Conversation

ffranr
Copy link
Contributor

@ffranr ffranr commented Aug 14, 2024

This is a follow up PR for this review comment: #1074 (comment)

This commit introduces the ChainHash RPC message, which contains two fields:

  • A raw byte representation of the hash, ideal for direct use in Go RPC calls.
  • A byte-reversed hexadecimal string representation of the hash, convenient for users who need to copy and paste the value, such as when passing it as a command-line argument.

@ffranr ffranr requested a review from Roasbeef August 14, 2024 19:06
@ffranr ffranr self-assigned this Aug 14, 2024
@ffranr
Copy link
Contributor Author

ffranr commented Aug 14, 2024

@guggero @Roasbeef, I'm wondering if there's a more general location where we can define this type of RPC message, so it can be utilized across all our RPC subpackages. Any ideas on that?

@ffranr ffranr requested a review from guggero August 14, 2024 19:08
@coveralls
Copy link

coveralls commented Aug 14, 2024

Pull Request Test Coverage Report for Build 10403975668

Details

  • 0 of 7 (0.0%) changed or added relevant lines in 1 file are covered.
  • 10 unchanged lines in 5 files lost coverage.
  • Overall coverage remained the same at 40.163%

Changes Missing Coverage Covered Lines Changed/Added Lines %
rpcserver.go 0 7 0.0%
Files with Coverage Reduction New Missed Lines %
rpcserver.go 1 0.0%
tapdb/addrs.go 2 79.04%
tappsbt/create.go 2 53.85%
asset/asset.go 2 81.54%
commitment/tap.go 3 84.7%
Totals Coverage Status
Change from base Build 10385622104: 0.0%
Covered Lines: 23684
Relevant Lines: 58970

💛 - Coveralls

Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

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

While I like the idea, see my inline comment for why we might not want to do it.
But happy to be overruled...

taprpc/taprootassets.proto Outdated Show resolved Hide resolved
taprpc/taprootassets.proto Outdated Show resolved Hide resolved
taprpc/taprootassets.proto Outdated Show resolved Hide resolved
This commit introduces the `ChainHash` RPC message, which contains two
fields:
- A raw byte representation of the hash, ideal for direct use in
  Go RPC calls.
- A byte-reversed hexadecimal string representation of the hash,
  convenient for users who need to copy and paste the value, such as
  when passing it as a command-line argument.
@ffranr ffranr force-pushed the rpc-hash-message-type branch from 8961804 to 139669b Compare August 15, 2024 12:44
@ffranr ffranr changed the title rpc: add BlockHash message with both bytes and string representations rpc: add ChainHash message with both bytes and string representations Aug 15, 2024
@ffranr ffranr requested a review from guggero August 15, 2024 13:05
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

@jharveyb jharveyb self-requested a review August 15, 2024 15:07
Copy link
Contributor

@jharveyb jharveyb left a comment

Choose a reason for hiding this comment

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

LGTM - wondering if we should swap this in in other relevant spots / RPC messages before the next major release?

Non-blocking.

@ffranr ffranr added this pull request to the merge queue Aug 15, 2024
@guggero
Copy link
Member

guggero commented Aug 15, 2024

LGTM - wondering if we should swap this in in other relevant spots / RPC messages before the next major release?

I think we don't want to change any APIs that have already been in a released version. So the field changed in this PR is the only one we can change without potentially breaking anyone's application.

But we should keep this in mind for future fields.

Merged via the queue into main with commit 9ba2a07 Aug 15, 2024
17 checks passed
@guggero guggero deleted the rpc-hash-message-type branch August 15, 2024 15:53
@dstadulis dstadulis added this to the v0.4.2 milestone Aug 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

5 participants