fix(ids): %x formatting of ID and ShortID
#3956
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the formatting behavior for ID and ShortID by implementing the fmt.Formatter interface, ensuring that the %x and %#x verbs produce the expected hex output instead of the cb58 encoding.
- Implemented the fmt.Formatter interface for both
IDandShortIDvia a shared generic function. - Added unit tests to verify the behavior with multiple format specifiers and a benchmark to demonstrate stack allocation in
%qformatting.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| ids/format_test.go | Added tests to confirm proper formatting for various specifiers. |
| ids/format.go | Implemented fmt.Formatter interface for ID and ShortID types. |
| @@ -0,0 +1,40 @@ | |||
| package ids | |||
There was a problem hiding this comment.
why not put this in id.go ? it's only 40 LOC and the other formatting stuff are there.
There was a problem hiding this comment.
It would also need to be in short.go and then the common implementation would be separated from at least one use case. Here they naturally stay together and provide the reader with context.
ids/format_test.go
Outdated
| for _, tt := range tests { | ||
| t.Run(tt.id.String(), func(t *testing.T) { | ||
| for format, want := range tt.want { | ||
| if got := fmt.Sprintf(format, tt.id); got != want { |
There was a problem hiding this comment.
I'm curious to know the use case for this PR. We don't pass ID into Sprintf but use the logger, and there we use zap.Stringer.
There was a problem hiding this comment.
In an EVM implementation ids.ID is typically derived from a common.Hash (both [32]byte), which is idiomatically rendered as hex. My errors that include a block hash/ID use %#x.
| // format implements the [fmt.Formatter] interface for [ID] and [ShortID]. | ||
| func format[T interface { | ||
| idForFormatting | ||
| ID | ShortID |
There was a problem hiding this comment.
Why do we constrain the types to be either ID or ShortID?
Why this should be merged
fmt.Sprintf("%x", ids.ID{})returns the hex encoding of thecb58encoding, not of the raw bytes.How this works
Implement the
fmt.Formatterinterface for bothids.IDandids.ShortID.How this was tested
Unit test of formatting with
%v,%sand%q(equivalent toString()), plus%xand%#x(equivalent toHex()).Need to be documented in RELEASES.md?
Not sure. Maybe. I dunno
¯\_(ツ)_/¯