feat(test): add more tests for the presets, and compat with iso-ucan #14
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the payload encoding system in the varsig library to better support iso-ucan compatibility. The changes implement a more sophisticated encoding scheme where some payload types (like EIP191) require multiple encoding segments, replacing the previous single-value approach.
- Refactored PayloadEncoding from uint64 constants to int enum with separate encoding segments
- Updated encoding/decoding logic to handle multi-segment encodings (especially EIP191 variants)
- Added comprehensive test coverage for all preset signature algorithms with round-trip validation
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| constant.go | Major refactoring of PayloadEncoding system with new multi-segment encoding logic |
| common.go | Added new ECDSA preset functions (ES256, ES256K, ES384, ES512, EIP191) |
| common_test.go | Comprehensive test suite for all presets with iso-ucan compatibility validation |
| varsig_test.go | Removed unused helper functions and imports |
| rsa.go | Updated to use new EncodePayloadEncoding function |
| eddsa.go | Updated to use new EncodePayloadEncoding function |
| ecdsa.go | Updated to use new EncodePayloadEncoding function |
Comments suppressed due to low confidence (1)
common_test.go:15
- The function name 'TestName' is not descriptive. It should be renamed to describe what it's testing, such as 'TestUvarintDecoding' or similar based on its purpose.
func TestName(t *testing.T) {
| case Version0: | ||
| return decodeEncodingInfoV0(payEnc) | ||
| switch seg1 { | ||
| case encodingSegmentVerbatim: |
There was a problem hiding this comment.
The cases which simply return the switched value and nil can be combined using the switch variable (2 typ.)
| } | ||
| } | ||
|
|
||
| func roundTrip[T varsig.Varsig](t *testing.T, in T, expEncHex string) T { |
There was a problem hiding this comment.
It seems like we should round-trip at least one varsig just to verify what what we put in is also what we get out. I'm thinking of test vectors that allow this (JSON?) similar to those provided for did:key eventually.
There was a problem hiding this comment.
That round-trip still happens, it's just moved into that unified table test.
Turns out, PayloadEncoding can be multiple values for EIP191, which required some painful changes.