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

chore: check blockhash/txhash length whether it is 64 #699

Merged
merged 12 commits into from
Oct 12, 2022

Conversation

tkxkd0159
Copy link
Member

@tkxkd0159 tkxkd0159 commented Oct 6, 2022

Keep the case where txhash is empty to convey more detailed messages to the user.

Description

Since the transaction(or block) hash is 32 bytes and the length is 64 in hex string, then the second input validation of the req.Hash is to check whether the length of the requested. In addition, the route was changed from lbm/base/ostracon/v1/blocks/{hash} to lbm/base/ostracon/v1/block/{hash}. Because grpc-gw handler can't distinguish path parameter with same prefix url.

The odd thing is that GetBlockByHash often fails with a Not Implemented error when requested as REST. There is no problem with gRPC. There seems to be a bug in grpc-gw as it often fails. (currently, the test passed in the latest commit and the test failed in the previous commit, but there is no code change other than the markdown modification)

Motivation and context

How has this been tested?

I conducted a unit test for both REST and gRPC

Screenshots (if appropriate):

Checklist:

  • I followed the contributing guidelines and code of conduct.
  • I have added a relevant changelog to CHANGELOG.md
  • I have added tests to cover my changes.
  • I have updated the documentation accordingly.
  • I have updated API documentation client/docs/swagger-ui/swagger.yaml

@Finschia Finschia deleted a comment from CLAassistant Oct 6, 2022
@tkxkd0159 tkxkd0159 marked this pull request as ready for review October 6, 2022 06:49
@CLAassistant
Copy link

CLAassistant commented Oct 6, 2022

CLA assistant check
All committers have signed the CLA.

@tkxkd0159 tkxkd0159 changed the title chore: check txhash length whether it is 64 chore: check block/txhash length whether it is 64 Oct 6, 2022
@tkxkd0159 tkxkd0159 changed the title chore: check block/txhash length whether it is 64 chore: check blockhash/txhash length whether it is 64 Oct 6, 2022
@codecov
Copy link

codecov bot commented Oct 6, 2022

Codecov Report

Merging #699 (437b36b) into main (df29ba8) will increase coverage by 0.16%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #699      +/-   ##
==========================================
+ Coverage   61.20%   61.37%   +0.16%     
==========================================
  Files         874      874              
  Lines       98546    98553       +7     
==========================================
+ Hits        60316    60483     +167     
+ Misses      34659    34454     -205     
- Partials     3571     3616      +45     
Impacted Files Coverage Δ
client/grpc/tmservice/query.pb.go 32.27% <ø> (+3.76%) ⬆️
client/grpc/tmservice/query.pb.gw.go 27.66% <ø> (ø)
client/grpc/tmservice/service.go 68.75% <100.00%> (+5.59%) ⬆️
x/auth/tx/service.go 70.05% <100.00%> (+0.36%) ⬆️
x/token/validation.go 70.90% <0.00%> (-14.55%) ⬇️
x/token/msgs.go 32.48% <0.00%> (ø)
x/collection/msgs.go 39.07% <0.00%> (+1.11%) ⬆️
crypto/keys/internal/ecdsa/privkey.go 84.21% <0.00%> (+1.75%) ⬆️
... and 3 more

client/grpc/tmservice/service.go Outdated Show resolved Hide resolved
client/grpc/tmservice/service.go Outdated Show resolved Hide resolved
client/grpc/tmservice/service.go Outdated Show resolved Hide resolved
client/grpc/tmservice/service_test.go Outdated Show resolved Hide resolved
@tkxkd0159 tkxkd0159 requested a review from zemyblue October 8, 2022 04:56
Copy link
Member

@zemyblue zemyblue left a comment

Choose a reason for hiding this comment

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

LGTM

client/grpc/tmservice/service.go Outdated Show resolved Hide resolved
x/auth/tx/service.go Outdated Show resolved Hide resolved
client/grpc/tmservice/service_test.go Show resolved Hide resolved
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.

6 participants