Skip to content
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

Use x/auth/client for querying Txs #8732

Merged
merged 9 commits into from
Mar 1, 2021
Merged

Conversation

amaury1093
Copy link
Contributor

@amaury1093 amaury1093 commented Mar 1, 2021

Description

companion PR of #8734.

#8734 solves the issue in the correct way, but introduces breaking changes. This PR solves the same issue, without intoroducing breaking changes, but with some duplicate code.

Closes: #8680
Closes: #8681


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.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

@codecov
Copy link

codecov bot commented Mar 1, 2021

Codecov Report

Merging #8732 (495e0dc) into release/v0.41.x (2a5818c) will increase coverage by 0.00%.
The diff coverage is 60.12%.

Impacted file tree graph

@@                Coverage Diff                @@
##           release/v0.41.x    #8732    +/-   ##
=================================================
  Coverage            61.93%   61.93%            
=================================================
  Files                  611      612     +1     
  Lines                35250    35408   +158     
=================================================
+ Hits                 21831    21930    +99     
- Misses               11136    11167    +31     
- Partials              2283     2311    +28     
Impacted Files Coverage Δ
baseapp/grpcserver.go 2.70% <0.00%> (+0.13%) ⬆️
store/rootmulti/store.go 66.05% <0.00%> (ø)
testutil/network/network.go 90.54% <0.00%> (ø)
x/staking/keeper/keeper.go 56.66% <ø> (-1.40%) ⬇️
x/staking/keeper/validator.go 80.52% <ø> (+0.03%) ⬆️
client/grpc_query.go 29.16% <40.00%> (+0.90%) ⬆️
x/auth/tx/xauthclient.go 47.54% <47.54%> (ø)
baseapp/grpcrouter.go 80.00% <50.00%> (-6.05%) ⬇️
x/auth/client/cli/tx_multisign.go 64.67% <68.46%> (+2.99%) ⬆️
store/cachekv/store.go 87.12% <71.42%> (-2.76%) ⬇️
... and 14 more

Comment on lines 1 to 6
// Package tx 's xauthclient.go file is copy-pasted from
// https://github.com/cosmos/cosmos-sdk/blob/v0.41.3/x/auth/client/query.go
// It is duplicated as to not introduce any breaking change in 0.41.4, see PR:
// TODO
// It is refactored and removed in the following PR:
// TODO
Copy link
Contributor Author

@amaury1093 amaury1093 Mar 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR fixes query Txs (by hash or by event) GRPC handlers, by filling out all required fields.

The thing is, all this logic is already implemented in x/auth/client/query.go, so the GRPC handlers could just call that file. Unfortunately, that creates an import cycle, see #8732 (comment)

To not introduce breaking changes, I copy-pasted the relevant functions from x/auth/client/query.go to this folder. We have duplicate code, and will have it on 0.41.4.

I also created #8734, which is the correct way imo to solve things, which will be on master and on 0.42+.

query := strings.Join(tmEvents, " AND ")

result, err := s.clientCtx.Client.TxSearch(ctx, query, false, &page, &limit, "")
result, err := queryTxsByEvents(s.clientCtx, req.Events, page, limit, "")
Copy link
Contributor Author

@amaury1093 amaury1093 Mar 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling authclient.QueryTxsByEvents is the correct way to do things, but it introduces an import cycle, so I just copy/pasted queryTxsByEvents to this package :(

@amaury1093 amaury1093 marked this pull request as ready for review March 1, 2021 14:38
@amaury1093 amaury1093 changed the base branch from master to release/v0.41.x March 1, 2021 14:42
@amaury1093 amaury1093 marked this pull request as draft March 1, 2021 14:44
@amaury1093 amaury1093 marked this pull request as ready for review March 1, 2021 15:07
@amaury1093 amaury1093 added this to the v0.41.4 milestone Mar 1, 2021
Copy link
Collaborator

@robert-zaremba robert-zaremba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good. Left few comments.

x/auth/tx/service.go Outdated Show resolved Hide resolved
@@ -194,7 +194,7 @@ func (s IntegrationTestSuite) TestGetTxEvents_GRPC() {
{
"with multi events",
&tx.GetTxsEventRequest{
Events: []string{"message.action=send", "message.module=bank"},
Events: []string{"message.action='send'", "message.module='bank'"},
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is ' required?

Copy link
Contributor Author

@amaury1093 amaury1093 Mar 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If i'm not mistaken, it's required by tendermint.

Somehow, in the gRPC handler, we added a logic saying that "if there's no ', we add a '". I think people are used to ', so I'm proposing to do exactly what tendermint expects, and remove the "add ' automatically" logic.

x/auth/tx/xauthclient.go Outdated Show resolved Hide resolved
}
any := p.AsAny()
return sdk.NewResponseResultTx(resTx, any, resBlock.Block.Time.Format(time.RFC3339)), nil
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is just code copy, right? Maybe you can add a source in a comment at the top of the file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's there already!

Copy link
Collaborator

@robert-zaremba robert-zaremba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved. Maybe we can also check the documentation and update it to mention that Apostrophe in event values is required?

@amaury1093
Copy link
Contributor Author

amaury1093 commented Mar 1, 2021

Maybe we can also check the documentation and update it to mention that Apostrophe in event values is required?

OK, I can make sure about that, #8736

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

missing error message when calling GetTxsEvent Missing timestamp in GetTxsEvent response
3 participants