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

Move QueryTx functions to x/auth/tx #8734

Merged
merged 9 commits into from
Mar 1, 2021
Merged

Conversation

amaury1093
Copy link
Contributor

@amaury1093 amaury1093 commented Mar 1, 2021

Description

closes: #8680
closes: #8681

It was confusing that the gRPC handler GetTxsEvent was in x/auth/tx, whereas the logic QueryTxsByEvents was in x/auth/client (same for querying tx by hash). I propose to put them together in x/auth/tx.


(on a related note: I find the 2 packages x/auth/client and x/auth/tx too similar. For example, x/auth/client exports the SignTx function, which could totally be in x/auth/tx. Maybe merging them would make things cleaner?)


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

@amaury1093 amaury1093 changed the title Move tx service implementation to x/auth/client Move QueryTx function to x/auth/tx Mar 1, 2021
@amaury1093 amaury1093 changed the title Move QueryTx function to x/auth/tx Move QueryTx functions to x/auth/tx Mar 1, 2021
@amaury1093 amaury1093 changed the base branch from am/8680-missing to master March 1, 2021 14:42
@codecov
Copy link

codecov bot commented Mar 1, 2021

Codecov Report

Merging #8734 (da84657) into master (5f2b90c) will increase coverage by 0.06%.
The diff coverage is 70.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8734      +/-   ##
==========================================
+ Coverage   61.38%   61.45%   +0.06%     
==========================================
  Files         673      673              
  Lines       38337    38306      -31     
==========================================
+ Hits        23535    23541       +6     
+ Misses      12334    12285      -49     
- Partials     2468     2480      +12     
Impacted Files Coverage Δ
x/auth/tx/query.go 47.54% <ø> (ø)
x/staking/client/rest/utils.go 0.00% <0.00%> (ø)
x/auth/client/cli/query.go 38.70% <50.00%> (ø)
x/auth/tx/service.go 69.35% <71.42%> (-2.69%) ⬇️
x/auth/client/rest/query.go 38.83% <100.00%> (ø)
x/gov/client/utils/query.go 25.72% <100.00%> (ø)
x/auth/tx/builder.go 76.37% <0.00%> (+0.78%) ⬆️

@amaury1093 amaury1093 marked this pull request as ready for review March 1, 2021 15:07
@amaury1093 amaury1093 added the C: gRPC Issues and PRs related to the gRPC service and HTTP gateway. label Mar 1, 2021
@alessio alessio merged commit a93edee into master Mar 1, 2021
@alessio alessio deleted the am/8732-remove-duplicate branch March 1, 2021 15:57
@amaury1093
Copy link
Contributor Author

amaury1093 commented Mar 1, 2021

i forgot to add a changelog entry. it's an API-breaking change, so it deserves one. #8736 (comment)

@amaury1093 amaury1093 mentioned this pull request Mar 1, 2021
7 tasks
@robert-zaremba
Copy link
Collaborator

robert-zaremba commented Mar 4, 2021

(on a related note: I find the 2 packages x/auth/client and x/auth/tx too similar. For example, x/auth/client exports the SignTx function, which could totally be in x/auth/tx. Maybe merging them would make things cleaner?)

x/auth needs a big refactor. I started it few months ago, but it took more then expected and didn't finish. Major problem is that other packages (anything not in x/...) modules depends on x/auth.

@alessio
Copy link
Contributor

alessio commented Mar 4, 2021

Major problem is that other packages (anything not in x/...) modules depends on x/auth.

It's a major one indeed. Nothing outside x/ must depend on anything under the x/ tree

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: gRPC Issues and PRs related to the gRPC service and HTTP gateway.
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