-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Fix fetch txs by height on legacy REST endpoint #7730
Conversation
04b3a0b
to
fe36f9c
Compare
return | ||
} | ||
|
||
if output.Empty() { | ||
rest.WriteErrorResponse(w, http.StatusNotFound, fmt.Sprintf("no transaction found with hash %s", hashHexStr)) | ||
} | ||
|
||
rest.PostProcessResponseBare(w, clientCtx, stdTx) | ||
rest.PostProcessResponseBare(w, clientCtx, output) |
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.
This happens on the query tx by hash endpoint. On master: we output a StdTx. In this PR, I changed to output a TxResponse.
This is a breaking change compared to master, but it's actually the same output as 0.39. I think TxResponse is better here.
x/auth/client/rest/rest_test.go
Outdated
s.Require().True(strings.Contains(string(txJSON), stdTx.Memo)) | ||
|
||
// We now fetch the tx on `/txs` route, filtering by `height` | ||
txJSON, err = rest.GetRequest(fmt.Sprintf("%s/txs?height=%d", val0.APIAddress, txRes.Height)) |
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.
Anyone knows why we can have both the ?height=
and the ?tx.height=
query params?
?tx.height
(posted by Zaki initially) now works correctly. However, ?height
returns 0 result, but maybe it means something else?
Test fails because I'm not sure what it's actually supposed to do.
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.
I removed testing ?height
for now. I am not sure if ?height=n
is supposed to:
- set the clientCtx height to
n
- query events with attribute
height=n
- or both
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.
ACK
Codecov Report
@@ Coverage Diff @@
## master #7730 +/- ##
==========================================
+ Coverage 54.31% 54.34% +0.03%
==========================================
Files 610 610
Lines 38621 38627 +6
==========================================
+ Hits 20976 20992 +16
+ Misses 15508 15492 -16
- Partials 2137 2143 +6 |
Description
closes #7721
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/
) or specification (x/<module>/spec/
)godoc
comments.Unreleased
section inCHANGELOG.md
Files changed
in the Github PR explorerCodecov Report
in the comment section below once CI passes