rpc: fix SendRawTransaction_Result.Reason json tag, add tag-hygiene tests - #75
Open
DHEBP wants to merge 1 commit into
Open
rpc: fix SendRawTransaction_Result.Reason json tag, add tag-hygiene tests#75DHEBP wants to merge 1 commit into
DHEBP wants to merge 1 commit into
Conversation
…ests
SendRawTransaction_Result.Reason was tagged `json:"string"` instead of
`json:"reason"`, so every DERO.SendRawTransaction response carries an
undocumented "string" key and a client reading "reason" finds nothing.
Impact today is nil rather than data loss: Reason is never assigned anywhere in
the tree, so the field is always "". The two soft-failure paths in
rpc_dero_sendrawtransaction.go report via result.Status, and real rejections
return a Go error that becomes a JSON-RPC error object, so the failure text a
client sees is unaffected either way. The tag is corrected rather than the field
removed; whether to populate it or delete it is an API decision for maintainers,
and populating it would mean changing the rejection branch to return a result
instead of an error, which IS a breaking change and should not ride along here.
rpc/jsontag_test.go adds three checks over the whole package, parsing the source
so no one has to remember to register a type:
- no json tag named after a Go builtin type (this is what the above defect
looks like mechanically: the field's TYPE written where its NAME belongs)
- no duplicate json tag within a struct, where one field silently never
reaches the wire and the winner depends on field order
- json tags are lowercase, matching the rest of the wire format
These run with `go test ./rpc/` -- no new dependency, no CI job required. The
first check fails on the unfixed tree and passes after, while the other two pass
in both states, so the suite is not merely failing everything.
This file contains hidden or 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
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.
SendRawTransaction_Result.Reason is tagged
json:"string"instead ofjson:"reason", so every DERO.SendRawTransaction response carries anundocumented "string" key and a client reading "reason" finds nothing.
Impact today is nil rather than data loss. Reason is never assigned anywhere in
the tree, so the field is always empty. The two soft-failure paths in
rpc_dero_sendrawtransaction.go report via result.Status, and real rejections
return a Go error that becomes a JSON-RPC error object, so the failure text a
client actually sees is unaffected either way.
I corrected the tag rather than removing the field. Whether to populate it or
delete it is an API decision for maintainers, and populating it would mean
changing the rejection branch to return a result instead of an error -- that
removes the JSON-RPC error object which is currently the only rejection signal
derohe's own wallet reads, so it is a breaking change and should not ride along
with a tag fix.
The second half is a guard so this class does not recur. rpc/jsontag_test.go
parses the package source (rather than using reflection) and asserts:
looks like mechanically: the field's TYPE written where its NAME belongs
reaches the wire and which one wins depends on field order
It runs under
go test ./rpc/. No new dependency and no CI job required.Verification: the first check fails on the unfixed tree, naming the exact
file:line and suggesting the correct tag, and passes after the fix. The other
two pass in both states, so the suite is not merely failing everything. I
re-confirmed this after rebasing by reintroducing the bad tag and watching the
check fire. go build ./... is clean and go test ./rpc/ passes.
Every type in this package is part of derod's or the wallet's public wire
format, and a wrong tag there is invisible at compile time and invisible to
tests that only exercise Go structs -- it only shows up as a field a client can
never read.