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.

Sign up for GitHub

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

Jump to bottom

Problem: Swagger UI does not include NFT APIs #533

Merged
merged 4 commits into from
May 7, 2021
Merged

Problem: Swagger UI does not include NFT APIs #533

merged 4 commits into from
May 7, 2021

Conversation

yihuang
Copy link
Collaborator

@yihuang yihuang commented May 6, 2021 •
edited
Loading

fixes #531

Solution:

  • vendor in swagger script and update the assets
  • port protobuf tools from cosmos-sdk
  • update swagger title

How to use

  • make proto-swagger-gen update swagger files
  • make update-swagger-docs update statik embed swagger assets
  • make proto-all format/lint protobuf files and gen golang code

👮🏻👮🏻👮🏻 !!!! REFERENCE THE PROBLEM YOUR ARE SOLVING IN THE PR TITLE AND DESCRIBE YOUR SOLUTION HERE !!!! DO NOT FORGET !!!! 👮🏻👮🏻👮🏻

PR Checklist:

  • Have you read the CONTRIBUTING.md?
  • Does your PR follow the C4 patch requirements?
  • Have you rebased your work on top of the latest master?
  • Have you checked your code compiles? (make)
  • Have you included tests for any non-trivial functionality?
  • Have you checked your code passes the unit tests? (make test)
  • Have you checked your code formatting is correct? (go fmt)
  • Have you checked your basic code style is fine? (golangci-lint run)
  • If you added any dependencies, have you checked they do not contain any known vulnerabilities? (go list -json -m all | nancy sleuth)
  • If your changes affect the client infrastructure, have you run the integration test?
  • If your changes affect public APIs, does your PR follow the C4 evolution of public contracts?
  • If your code changes public APIs, have you incremented the crate version numbers and documented your changes in the CHANGELOG.md?
  • If you are contributing for the first time, please read the agreement in CONTRIBUTING.md now and add a comment to this pull request stating that your PR is in accordance with the Developer's Certificate of Origin.

Thank you for your code, it's appreciated! :)

@codecov
Copy link

codecov bot commented May 6, 2021 •
edited
Loading

Codecov Report

Merging #533 (ad60814) into master (39ecd2d) will increase coverage by 5.09%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #533      +/-   ##
==========================================
+ Coverage   11.15%   16.24%   +5.09%     
==========================================
  Files          36       69      +33     
  Lines        4804     9127    +4323     
==========================================
+ Hits          536     1483     +947     
- Misses       4182     7208    +3026     
- Partials       86      436     +350
Flag Coverage Δ
integration_tests 12.21% <ø> (?)
unit_tests 11.15% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
app/app.go 17.98% <ø> (+17.26%) ⬆️
app/docs/statik/statik.go 100.00% <ø> (ø)
x/nft/keeper/invariants.go 7.40% <0.00%> (-49.74%) ⬇️
x/nft/types/msgs.go 13.43% <0.00%> (-46.57%) ⬇️
x/nft/keeper/querier.go 7.90% <0.00%> (-17.10%) ⬇️
x/nft/types/codec.go 0.00% <0.00%> (ø)
x/nft/client/cli/flags.go 100.00% <0.00%> (ø)
app/params/proto.go 100.00% <0.00%> (ø)
x/supply/client/cli/query.go 65.38% <0.00%> (ø)
x/chainmain/client/cli/tx.go 0.00% <0.00%> (ø)
... and 40 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 39ecd2d...ad60814. Read the comment docs.

calvinaco
calvinaco reviewed May 6, 2021
View reviewed changes
app/docs/config.json Outdated Show resolved Hide resolved
tomtau
tomtau reviewed May 6, 2021
View reviewed changes
Copy link
Contributor

@tomtau tomtau left a comment

Choose a reason for hiding this comment

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

@yihuang one thing I remember I think @cdc-Hitesh mentioned was that Swagger examples used a wrong bech32 address hrp -- can this be fixed too?

app/docs/statik/init.go
@@ -0,0 +1,3 @@
package statik

Copy link
Contributor

Choose a reason for hiding this comment

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

lol statik.go file has 1.6MB

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah, it embed the whole app/docs/swagger-ui directory.

leejw51crypto
leejw51crypto approved these changes May 6, 2021
View reviewed changes
Copy link
Contributor

@leejw51crypto leejw51crypto left a comment

Choose a reason for hiding this comment

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

lgtm

@yihuang
Copy link
Collaborator Author

yihuang commented May 6, 2021 •
edited
Loading

@yihuang one thing I remember I think @cdc-Hitesh mentioned was that Swagger examples used a wrong bech32 address hrp -- can this be fixed too?

there are many cosmos appeared in the swagger.yaml file, do we really want to patch those? 😄

@tomtau
Copy link
Contributor

tomtau commented May 7, 2021

@yihuang one thing I remember I think @cdc-Hitesh mentioned was that Swagger examples used a wrong bech32 address hrp -- can this be fixed too?

there are many cosmos appeared in the swagger.yaml file, do we really want to patch those? 😄

There's no problem with the protobuf types etc. -- but for examples payloads... if one tries to use the example payload, it'll fail with an error that the address validation failed. Maybe can open an issue for that

@yihuang
Copy link
Collaborator Author

yihuang commented May 7, 2021

@yihuang one thing I remember I think @cdc-Hitesh mentioned was that Swagger examples used a wrong bech32 address hrp -- can this be fixed too?

there are many cosmos appeared in the swagger.yaml file, do we really want to patch those? 😄

There's no problem with the protobuf types etc. -- but for examples payloads... if one tries to use the example payload, it'll fail with an error that the address validation failed. Maybe can open an issue for that

done, seems manageable.

yihuang and others added 3 commits May 7, 2021 11:13
@yihuang @calvinaco
Problem: Swagger UI does not include NFT APIs
44246c6 
fixes crypto-org-chain#531

Solution:
- vendor in swagger script and update the assets
- port protobuf tools from cosmos-sdk
- update swagger title

Co-authored-by: Calvin Lau <38898718+calvinaco@users.noreply.github.com>
@yihuang
patch swagger document
75fbf4c
@yihuang
try fix statesync integration test
e3c69e4
@tomtau tomtau self-requested a review May 7, 2021 05:11
@tomtau tomtau merged commit e528844 into crypto-org-chain:master May 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@calvinaco calvinaco calvinaco left review comments

@tomtau tomtau tomtau approved these changes

@devashishdxt devashishdxt devashishdxt approved these changes

@leejw51crypto leejw51crypto leejw51crypto approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Problem: Swagger UI does not include NFT APIs
5 participants
@yihuang @tomtau @calvinaco @devashishdxt @leejw51crypto