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

(BUG FIX) Fix order submission crash due to missing "exchange" parameter #468

Merged
merged 7 commits into from
Mar 14, 2020

Conversation

woshidama323
Copy link
Contributor

PR Description

RPC server crashed when requesting submit by using gctcli

Fixes # (issue)

Type of change

Please delete options that are not relevant and add an x in [] as item is complete.

  • Bug fix (non-breaking change which fixes an issue)

How has this been tested

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration and
also consider improving test coverage whilst working on a certain feature or package.

  • gctcli submitorder binance EOS-USDT SELL SPOT 1

Checklist

  • [x ] My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • 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
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally and on Travis with my changes
  • Any dependent changes have been merged and published in downstream modules

@codecov
Copy link

codecov bot commented Mar 11, 2020

Codecov Report

Merging #468 into master will decrease coverage by 0.01%.
The diff coverage is 63.63%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #468      +/-   ##
==========================================
- Coverage   45.50%   45.49%   -0.02%     
==========================================
  Files         210      210              
  Lines       48818    48826       +8     
==========================================
- Hits        22217    22213       -4     
- Misses      24343    24351       +8     
- Partials     2258     2262       +4     
Impacted Files Coverage Δ
engine/rpcserver.go 0.00% <0.00%> (ø)
exchanges/bitmex/bitmex_wrapper.go 53.16% <100.00%> (ø)
exchanges/exchange.go 96.62% <100.00%> (ø)
exchanges/order/orders.go 98.51% <100.00%> (+0.01%) ⬆️
database/models/postgres/audit_event.go 51.84% <0.00%> (-1.31%) ⬇️
exchanges/websocket/wshandler/wshandler.go 71.69% <0.00%> (-0.92%) ⬇️
exchanges/lakebtc/lakebtc_websocket.go 60.32% <0.00%> (+1.63%) ⬆️

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 25a23af...96c23ea. Read the comment docs.

Copy link

@codelingo codelingo bot left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

@codelingo codelingo bot left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

@codelingo codelingo bot left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

@codelingo codelingo bot left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

@codelingo codelingo bot left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@xtda xtda left a comment

Choose a reason for hiding this comment

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

tested nice pick up thanks! I don't remember this coming up during testing of the pr that changed it odd

Approved with one comment

@@ -898,6 +904,8 @@ func (s *RPCServer) CancelOrder(ctx context.Context, r *gctrpc.CancelOrderReques
ID: r.OrderId,
Side: order.Side(r.Side),
WalletAddress: r.WalletAddress,
Pair : currency.NewPairFromStrings(r.Pair.Base,r.Pair.Quote),

})

return &gctrpc.CancelOrderResponse{}, err
Copy link
Contributor

Choose a reason for hiding this comment

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

if you feel comfortable with protobuf/gRPC you could also update the CancelOrderResponse{} message here to pass back a confirmation but its kind of out of scope of the original fix and I am happy to open up a PR for it at a later time

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes you can , let's fix them as quickly as possible

Copy link
Collaborator

Choose a reason for hiding this comment

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

We can do this in a separate PR

@xtda xtda added bug review me This pull request is ready for review labels Mar 11, 2020
@xtda xtda changed the title Fix the crash issue when sending the submit request to the rpc server (BUG FIX) Fix order submission crash due to missing "exchange" parameter Mar 11, 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.

tACK! Thanks for this fix

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.

Great find! Just one nit.

@@ -898,6 +904,8 @@ func (s *RPCServer) CancelOrder(ctx context.Context, r *gctrpc.CancelOrderReques
ID: r.OrderId,
Side: order.Side(r.Side),
WalletAddress: r.WalletAddress,
Pair : currency.NewPairFromStrings(r.Pair.Base,r.Pair.Quote),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Go formatting issues picked up by our linter:
image

Please run gofmt -w rpcserver.go & push up the fix.

@@ -898,6 +904,7 @@ func (s *RPCServer) CancelOrder(ctx context.Context, r *gctrpc.CancelOrderReques
ID: r.OrderId,
Side: order.Side(r.Side),
WalletAddress: r.WalletAddress,
Pair: currency.NewPairFromStrings(r.Pair.Base, r.Pair.Quote),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have adopted the suggestion of gofmt

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment on lines +809 to +813

if err != nil {
return &gctrpc.SubmitOrderResponse{}, err
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here, I also suggest merging it into the master branch. It would panic if &gctrpc.SubmitOrderResponse is nil

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.

Thanks for your PR! ACK

@thrasher- thrasher- merged commit 99e8d2f into thrasher-corp:master Mar 14, 2020
MadCozBadd pushed a commit to MadCozBadd/gocryptotrader that referenced this pull request Mar 24, 2020
…ter (thrasher-corp#468)

* fix the return problem

* fix the return problem

* add the mandatory parameter

* fix the symbol empity problem

* fix the symbol empty problem

* add Pair parameter for order.Cancel parameters

* follow the gofmt rule

Co-authored-by: MK <mk@MKdeMacBook-Pro.local>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug review me This pull request is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants