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

fix(asset): 🔊 Metadata: Debug includes all fields #4207

Merged
merged 1 commit into from
Apr 15, 2024

Conversation

cratelyn
Copy link
Contributor

@cratelyn cratelyn commented Apr 15, 2024

fixes #4190.

The Debug impl for asset::Metadata throws away all data except for
the base denom. This is extremely inconvenient for debugging.

this commit updates Metadata's impl of std::fmt::Debug so that it includes all of the fields in the Inner structure.

note for review: #4190 didn't explicitly mention which fields to include, so i have included all of them as a starting point. if we should find a middle ground, i am open and interested in hearing about that!

issue ticket number and link

#4190

checklist before requesting a review

  • If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason:

    debug representations should not affect consensus.

@cratelyn cratelyn added the A-staking Area: Design and implementation of staking and delegation label Apr 15, 2024
@cratelyn cratelyn added this to the Sprint 4 milestone Apr 15, 2024
@cratelyn cratelyn self-assigned this Apr 15, 2024
@cratelyn cratelyn requested a review from hdevalence April 15, 2024 15:04
fixes #4190.

> The `Debug` impl for `asset::Metadata` throws away all data except for
the base denom. This is extremely inconvenient for debugging.

- #4190

---

this commit updates `Metadata`'s impl of `std::fmt::Debug` so that it
includes all of the fields in the `Inner` structure.

note for review: #4190 didn't explicitly mention which fields to
include, so i have included all of them as a starting point. if we
should find a middle ground, i am open and interested in hearing about
that!
@cratelyn cratelyn force-pushed the kate/metadata-debug-repr-includes-all-fields branch from b5fc00a to 335d305 Compare April 15, 2024 15:05
Copy link
Member

@hdevalence hdevalence left a comment

Choose a reason for hiding this comment

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

LGTM!

If we notice, as a result of this change, that there are places in the code where we're putting a ton of info in a tracing event (because we did ?metadata or something, relying on current behavior), we should change those to do the base denom or ID as appropriate

@hdevalence hdevalence merged commit 2d9c784 into main Apr 15, 2024
8 checks passed
@hdevalence hdevalence deleted the kate/metadata-debug-repr-includes-all-fields branch April 15, 2024 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-staking Area: Design and implementation of staking and delegation
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

asset::Metadata Debug impl throws away all data except base denom
2 participants