-
Notifications
You must be signed in to change notification settings - Fork 817
fix(crypto/secp256k1): UnmarshalText #4419
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes a bug in the secp256k1 private key unmarshaling logic where UnmarshalText was incorrectly delegating to UnmarshalJSON, causing failures when round-trip marshaling text representations since MarshalText returns unquoted strings while UnmarshalJSON expects quoted strings.
- Refactored unmarshaling logic by extracting common functionality into a new
unmarshalTexthelper method - Fixed
UnmarshalTextto properly handle unquoted text input instead of delegating to JSON unmarshaling - Added regression test to verify round-trip text marshaling works correctly
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| utils/crypto/secp256k1/secp256k1.go | Refactored unmarshaling logic to fix text unmarshaling bug and extract common functionality |
| utils/crypto/secp256k1/secp256k1_test.go | Added regression test for text marshaling round-trip functionality |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Why this should be merged
Normally,
x = UnmarshalText(MarshalText(x)). However, this currently fails becauseUnmarshalText()expects a quoted string (as it callsUnmarshalJSON()and JSON marshaling uses quoted strings) whileMarshalText()returns a string without quotes.How this works
Factors out the common logic of
UnmarshalText()andUnmarshalJSON()intounmarshalText()(which expects an unquoted string).UnmarshalJSON()now handles its own quoted string logic.How this was tested
Added a regression test.
Need to be documented in RELEASES.md?
No.