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

[Access] Created new gRPC methods for GetAccountBalance and GetAccountKeys. #6144

Conversation

AndriiDiachuk
Copy link
Contributor

@AndriiDiachuk AndriiDiachuk commented Jun 24, 2024

In this PR new gRPC methods GetAccountBalance and GetAccountKeys were added.

Closing issue: #5999
Related PRs: onflow/flow#1475 onflow/flow-emulator#707

@codecov-commenter
Copy link

codecov-commenter commented Jun 24, 2024

Codecov Report

Attention: Patch coverage is 8.23666% with 791 lines in your changes missing coverage. Please review.

Project coverage is 41.39%. Comparing base (b63957c) to head (2a695a6).

Files Patch % Lines
engine/access/mock/access_api_client.go 0.00% 144 Missing ⚠️
engine/access/mock/access_api_server.go 0.00% 108 Missing ⚠️
access/mock/api.go 0.00% 106 Missing ⚠️
access/handler.go 0.00% 102 Missing ⚠️
engine/access/apiproxy/access_api_proxy.go 0.00% 84 Missing ⚠️
engine/access/rpc/backend/backend_accounts.go 47.18% 51 Missing and 24 partials ⚠️
engine/execution/computation/query/executor.go 0.00% 37 Missing ⚠️
...ngine/execution/computation/query/mock/executor.go 0.00% 35 Missing ⚠️
module/execution/mock/script_executor.go 0.00% 35 Missing ⚠️
fvm/environment/mock/account_info.go 0.00% 18 Missing ⚠️
... and 4 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6144      +/-   ##
==========================================
- Coverage   41.58%   41.39%   -0.19%     
==========================================
  Files        1976     1976              
  Lines      139757   140591     +834     
==========================================
+ Hits        58112    58201      +89     
- Misses      75609    76335     +726     
- Partials     6036     6055      +19     
Flag Coverage Δ
unittests 41.39% <8.23%> (-0.19%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@peterargue peterargue left a comment

Choose a reason for hiding this comment

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

overall this is looking good. added a couple comments related to my comments in the proto changes

engine/access/rpc/backend/backend_accounts.go Outdated Show resolved Hide resolved
engine/access/rpc/backend/backend_accounts.go Outdated Show resolved Hide resolved
engine/access/rpc/backend/backend_accounts.go Outdated Show resolved Hide resolved
access/handler.go Outdated Show resolved Hide resolved
access/handler.go Outdated Show resolved Hide resolved
@AndriiDiachuk AndriiDiachuk marked this pull request as ready for review June 27, 2024 13:33
Copy link
Contributor

@peterargue peterargue left a comment

Choose a reason for hiding this comment

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

replied to your question about getting the available balance when using execution-only-mode. sorry for the churn on that request.

access/api.go Outdated Show resolved Hide resolved
engine/access/rpc/backend/backend_accounts.go Outdated Show resolved Hide resolved
engine/access/rpc/backend/backend_accounts.go Outdated Show resolved Hide resolved
engine/access/rpc/backend/backend_accounts.go Outdated Show resolved Hide resolved
engine/access/rpc/backend/backend_accounts.go Outdated Show resolved Hide resolved
engine/access/rpc/backend/backend_accounts.go Outdated Show resolved Hide resolved
fvm/environment/env.go Outdated Show resolved Hide resolved
fvm/fvm.go Outdated Show resolved Hide resolved
module/execution/scripts.go Outdated Show resolved Hide resolved
Copy link
Contributor

@peterargue peterargue left a comment

Choose a reason for hiding this comment

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

added a few more comments. after this I think it's ready to go.

access/handler.go Outdated Show resolved Hide resolved
access/handler.go Outdated Show resolved Hide resolved
access/handler.go Outdated Show resolved Hide resolved
access/handler.go Outdated Show resolved Hide resolved
access/handler.go Show resolved Hide resolved
access/handler.go Show resolved Hide resolved
engine/access/rpc/backend/backend_accounts.go Outdated Show resolved Hide resolved
engine/access/rpc/backend/backend_accounts_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@peterargue peterargue left a comment

Choose a reason for hiding this comment

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

Looks good. @Guitarheroua can you take a look as well. after we get the merged emulator commit added, I'll approve and merge.

Copy link
Collaborator

@Guitarheroua Guitarheroua left a comment

Choose a reason for hiding this comment

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

Looks good! Just a minor comment about documentation.

@peterargue
Copy link
Contributor

@AndriiDiachuk I merged the emulator PR, please update this PR with the commit and I'll approve and merge

@AndriiDiachuk
Copy link
Contributor Author

@AndriiDiachuk I merged the emulator PR, please update this PR with the commit and I'll approve and merge

Made the changes, but please check if I made any mistakes.

Copy link
Contributor

@janezpodhostnik janezpodhostnik left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Great work.

I'll change some of the types in #6201

@peterargue peterargue added this pull request to the merge queue Jul 12, 2024
Merged via the queue into onflow:master with commit d0d12c8 Jul 12, 2024
55 checks passed
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.

5 participants