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

Problem: unnecessary use of REST API instead of grpc-web (get_account_balance part) #663

Conversation

stevenatcrypto
Copy link
Collaborator

@stevenatcrypto stevenatcrypto commented Nov 6, 2022

Part of issue #511 (will fix get_account_details in another PR)

Replace REST API with grpc-web for get_account_balance functions.

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 main?
  • Have you checked your code compiles? (cargo build)
  • Have you included tests for any non-trivial functionality?
  • Have you checked your code passes the unit tests? (cargo test)
  • Have you checked your code formatting is correct? (cargo fmt -- --check --color=auto)
  • Have you checked your basic code style is fine? (cargo clippy)
  • If you added any dependencies, have you checked they do not contain any known vulnerabilities? (cargo audit)
  • 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.

@stevenatcrypto stevenatcrypto changed the title Problem: unnecessary use of REST API instead of grpc-web (part of get_account_balance) Problem: unnecessary use of REST API instead of grpc-web (get_account_balance part) Nov 6, 2022
@codecov-commenter
Copy link

codecov-commenter commented Nov 6, 2022

Codecov Report

Base: 43.83% // Head: 43.97% // Increases project coverage by +0.14% 🎉

Coverage data is based on head (2b7928d) compared to base (9bd0ca6).
Patch coverage: 0.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #663      +/-   ##
==========================================
+ Coverage   43.83%   43.97%   +0.14%     
==========================================
  Files          45       46       +1     
  Lines        8968     8939      -29     
==========================================
  Hits         3931     3931              
+ Misses       5037     5008      -29     
Impacted Files Coverage Δ
bindings/cpp/src/lib.rs 0.14% <0.00%> (+<0.01%) ⬆️
common/src/node/cosmos_sdk.rs 0.00% <0.00%> (ø)
common/src/node/cosmos_sdk/balance_query.rs 0.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@stevenatcrypto stevenatcrypto marked this pull request as draft November 7, 2022 01:35
@stevenatcrypto stevenatcrypto force-pushed the feat/replace-balance-rest-api-with-grpc-web branch 2 times, most recently from 8be7690 to 2f5f85e Compare November 13, 2022 04:03
@stevenatcrypto stevenatcrypto force-pushed the feat/replace-balance-rest-api-with-grpc-web branch from 5a49df6 to 1595083 Compare November 20, 2022 02:25
@stevenatcrypto stevenatcrypto marked this pull request as ready for review November 20, 2022 08:39
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.

looks ok, but some binding examples (android, JS/Wasm...) may still have old URLs?

example/js-example/index.js Outdated Show resolved Hide resolved
@tomtau tomtau merged commit f0efa3b into crypto-com:main Nov 22, 2022
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.

4 participants