Skip to content

Write ChannelIds out as hex in Debug output #3306

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

Merged
merged 3 commits into from
Sep 10, 2024

Conversation

TheBlueMatt
Copy link
Collaborator

ChannelIds are almost always referenced as hex, so having debug
output print the raw bytes is somewhat annoying. Instead, we should
dump them as hex the same way we do for Display.

This uses the hex_conservative impl_fmt_macros which does all
the work for us, like we use for lightning_types.

We do this for `Payment*` in `lightning-types` and its needed for
the `hex_conservaitve` `impl_fmt_traits` macro which we'll use in
the next commit.
`ChannelId`s are almost always referenced as hex, so having debug
output print the raw bytes is somewhat annoying. Instead, we should
dump them as hex the same way we do for `Display`.

This uses the `hex_conservative` `impl_fmt_macros` which does all
the work for us, like we use for `lightning_types`.
To remove the now-redundant `hex_conservative` explicit dependency.
Copy link

codecov bot commented Sep 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.62%. Comparing base (eba329c) to head (98d15f2).
Report is 6 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3306      +/-   ##
==========================================
- Coverage   89.63%   89.62%   -0.01%     
==========================================
  Files         126      126              
  Lines      102166   102166              
  Branches   102166   102166              
==========================================
- Hits        91572    91564       -8     
- Misses       7869     7878       +9     
+ Partials     2725     2724       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

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

While we're here, can we introduce a UserChannelId type adding the same feature?

@TheBlueMatt
Copy link
Collaborator Author

Do we want user channel IDs to print as hex? One intended use-case for them is database row IDs, which presumably users don't want to see as hex.

@tnull
Copy link
Contributor

tnull commented Sep 9, 2024

Do we want user channel IDs to print as hex? One intended use-case for them is database row IDs, which presumably users don't want to see as hex.

Hm, well, not necessarily as hex, but IMO UserChannelId(340282366920938463463374607431768211455) would already improve readability in logs (and having the type live in LDK makes sense).

@TheBlueMatt
Copy link
Collaborator Author

I'm not sure I understand how UserChannelId() improves logs as long as the context is clear. eg in debug output you get things like Struct { .., user_channel_id: 123456789, .. }, which gives pretty clear context. If there's some logs that don't have context we should probably add context rather than wrap, IMO.

@tnull
Copy link
Contributor

tnull commented Sep 10, 2024

I'm not sure I understand how UserChannelId() improves logs as long as the context is clear. eg in debug output you get things like Struct { .., user_channel_id: 123456789, .. }, which gives pretty clear context. If there's some logs that don't have context we should probably add context rather than wrap, IMO.

Alright. Besides the display/debugging preferences, I'd generally prefer to use a dedicated type parallel to ChannelId in the API rather than a u128 (which, for example, doesn't work well on all platforms/languages). FWIW, LDK Node already has such a type, but IMO it would make sense to use it upstream as well. In any case, this seems to be a separate discussion which shouldn't hold up this PR.

@tnull tnull merged commit 479654b into lightningdevkit:main Sep 10, 2024
17 of 21 checks passed
@TheBlueMatt
Copy link
Collaborator Author

'd generally prefer to use a dedicated type parallel to ChannelId in the API rather than a u128 (which, for example, doesn't work well on all platforms/languages).

Mmm, yea. I generally dislike the proliferation of newtypes so prefer to only use them sparingly where it really makes sense (eg because we have a million [u8; 32] types flying around), but I admit the lack of support in various languages is a strong argument for doing it here.

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.

4 participants