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

engine/gRPC: Add update account info to grpc interface #602

Merged
merged 5 commits into from
Nov 30, 2020

Conversation

mshogin
Copy link
Contributor

@mshogin mshogin commented Nov 26, 2020

PR Description

This patch add support of UpdateAccountInfo to the GRPC server.
It is useful to have actual account state for the preliminary validation of the bid request.
Without such possibility we have to send requests to the exchange and either use completed orders for keeping account up to date or handle error messages from the exchange.

Type of change

  • [x ] New feature (non-breaking change which adds functionality)

How has this been tested

Added tests to the engine/rpcserver_test.go

Checklist

  • [ x] My code follows the style guidelines of this project
  • [ x] I have performed a self-review of my own code
  • [ x] I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation and regenerated documentation via the documentation tool
  • [ x] My changes generate no new warnings
  • [x ] I have added tests that prove my fix is effective or that my feature works
  • [ x] New and existing unit tests pass locally and on Travis with my changes (there are couple of broken tests in the upstream/master)
  • Any dependent changes have been merged and published in downstream modules

Copy link
Collaborator

@shazbert shazbert left a comment

Choose a reason for hiding this comment

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

Nice update 👍

I have tested exchanges I have keys on:

  • Binance
  • Kraken
  • Bitfinex
  • Bitmex
  • Coinut
  • HitBTC
  • Okcoin
  • Okex

Found some Out Of Scope Issues:

  1. Appending to currency list without any balance is very verbose console side from GRPC
  2. go run ./... updateaccountinfo coinut
    2020/11/26 15:39:06 rpc error: code = Unknown desc = COINUT websocket connection: timeout waiting for response with signature: 4868563
    exit status 1
  3. {
    "currency": "XRP",
    "total_value": 1234.1234 // Think this needs to change to amount in okex and okcoin.
    }

Thank you very much for opening this PR 🎉


r, err := s.GetAccountInfo(context.Background(), &gctrpc.GetAccountInfoRequest{Exchange: fakePassExchange})
if err != nil {
t.Errorf("TestGetAccountInfo: Failed to get account info: %w", err)
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can do t.Fatalf saves you doing return afterward.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

replaced

exchanges/lbank/lbank_types.go Show resolved Hide resolved

func cmpAccounts(t *testing.T, exp []account.SubAccount, act []*gctrpc.Account) {
if len(act) != len(exp) {
t.Errorf("TestGetAccountInfo: Unexpected number of accouns in the response: exp=%d, act=%d", len(exp), len(act))
Copy link
Collaborator

Choose a reason for hiding this comment

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

accouns -> accounts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@shazbert shazbert added the review me This pull request is ready for review label Nov 26, 2020
Copy link
Collaborator

@gloriousCode gloriousCode left a comment

Choose a reason for hiding this comment

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

Thank you mshogin for contributing to the project! Thank you for filling in some missing RPC functionality. Thank you as well for patching up LBank. I've tested it and it works. I've just got a few comments.

engine/fake_exchange_test.go Outdated Show resolved Hide resolved
engine/fake_exchange_test.go Outdated Show resolved Hide resolved
engine/fake_exchange_test.go Outdated Show resolved Hide resolved
engine/fake_exchange_test.go Outdated Show resolved Hide resolved
@gloriousCode
Copy link
Collaborator

gloriousCode commented Nov 27, 2020

Sweet, thanks for making those changes so quickly! One more thing I've noticed that our build is having linter issues:
https://ci.appveyor.com/project/thrasher-/gocryptotrader-g5pk0/builds/36534791#L55
Would you be able to fix those up too?

@mshogin
Copy link
Contributor Author

mshogin commented Nov 27, 2020

sure

Copy link
Collaborator

@gloriousCode gloriousCode left a comment

Choose a reason for hiding this comment

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

tACK! Thanks for making those changes and thank you for your contribution @mshogin!

Copy link
Contributor

@MadCozBadd MadCozBadd left a comment

Choose a reason for hiding this comment

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

tested couple of exchanges n works just fine!! tACK

Copy link
Collaborator

@shazbert shazbert left a comment

Choose a reason for hiding this comment

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

tACK - Thanks! 🎉 🙌

@thrasher- thrasher- changed the title Add update account info to grpc interface engine/gRPC: Add update account info to grpc interface Nov 30, 2020
Copy link
Collaborator

@thrasher- thrasher- left a comment

Choose a reason for hiding this comment

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

tACK, thanks @mshogin for this new feature plus also for resolving the lbank OB issue!

./gctcli updateaccountinfo --exchange=binance
{
 "exchange": "Binance",
 "accounts": [
  {
   "currencies": [
    {
     "currency": "BTC",
     "total_value": xxx
...

@thrasher- thrasher- merged commit ba4ac4f into thrasher-corp:master Nov 30, 2020
@mshogin
Copy link
Contributor Author

mshogin commented Nov 30, 2020

Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review me This pull request is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants