Decouple from stellar/go monorepo #9
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
leighmcculloch
changed the title
leighmcculloch
commented
Feb 27, 2025
Comment on lines
-37
to
-60
| func ConvertBytes(xdr interface{}, field []byte) (json.RawMessage, error) { | ||
| if len(field) == 0 { | ||
| return []byte(""), nil | ||
| } | ||
|
|
||
| xdrTypeName := reflect.TypeOf(xdr).Name() | ||
| return convertAnyBytes(xdrTypeName, field) | ||
| } | ||
|
|
||
| // ConvertInterface takes a valid XDR object (`xdr`) and returns | ||
| // the raw JSON-formatted serialization of that object. If there is an | ||
| // error, it returns an empty string. | ||
| // | ||
| // Unlike `ConvertBytes`, the value here needs to be valid and | ||
| // serializable. | ||
| func ConvertInterface(xdr encoding.BinaryMarshaler) (json.RawMessage, error) { | ||
| xdrTypeName := reflect.TypeOf(xdr).Name() | ||
| data, err := xdr.MarshalBinary() | ||
| if err != nil { | ||
| return []byte(""), errors.Wrapf(err, "failed to serialize XDR type '%s'", xdrTypeName) | ||
| } | ||
|
|
||
| return convertAnyBytes(xdrTypeName, data) | ||
| } |
There was a problem hiding this comment.
ConvertBytes and ConvertInterface behave differently:
ConvertBytesreturns an empty string ("") if the[]byteis length zero.ConvertInterfacereturns an error if the[]byteis length zero, because the XDR decoder will error in that case for all XDR types.
@stellar/platform-eng Which is the correct behaviour? I think one must be a bug since both provide a different interface into the same capabilities.
There's a test at least that expects ConvertBytes to return an empty string, so seems some thought went into a use case that wanted an empty string.
Imo it'd be better if the function returned an error, because any other XDR decoder would return an error in this case, and to not return an error is silently hiding what is probably a problem. A system using this library can always translate the error into an empty string if it wants to hide errors.
Thoughts?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What
Remove ConvertBytes/Interface methods and add XDR type string constants.
Why
This is a follow up to:
The current exported interface with ConvertBytes and ConvertInterface encourages using the types defined outside the package, i.e. from the Go monorepo, as a name enum of sorts. Those types happen to be named the same, which is lucky given that if the Go types were named according to Go naming conventions they wouldn't match the Rust names encoded in the stellar-xdr Rust crate. The coupling is sort of convenient when using the stellar/go/xdr package, but otherwise the interface is very unintuitive.
Instead of the approach taken today, where another packages types are used as an enum, this package could provide a list of constants for all the types, and that would provide the same amount of certainty if folks were wanting to avoid having hardcoded strings.
Close #4
Notes
Generation of Type Constants
The constants are generated using the stellar-xdr Rust crate, so that the list of constants will always be up to date and match the underlying crate.
Rename of Convert* function to Decode
The function is renamed to align with the naming of the same operation in the
stellar-cli xdrcommand, and thejs-stellar-xdr-jsonpackage's exported interface. Some common nomenclature is helpful when maintaining a series of libraries and tools that export the same functionality.Todo
Merging
This PR should be merged after the PR below so that CI is in place: