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

(WIP): Withdraw additional functionality #409

Draft
wants to merge 34 commits into
base: master
from

Conversation

@xtda
Copy link
Member

xtda commented Jan 6, 2020

PR Description

This PR adds additional features to the withdraw functionality to GCT

Current working and planned features:

  • Validation for Crypto & Fiat Requests
  • New withdraw request submission manager in engine
  • monitor and track withdrawal request life cycle and update correct portfolio managers
  • Database tables and tracking for created withdrawal requests
  • Ability to track & search previous requests via CLI
  • submit withdrawal request via CLI (for both crypto and fiat)

Type of change

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

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.

  • go test ./... -race
  • golangci-lint run

Checklist

  • 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
xtda added 8 commits Dec 20, 2019
…ion system
@xtda xtda added the enhancement label Jan 6, 2020
@xtda xtda self-assigned this Jan 6, 2020
Copy link

codelingo bot left a comment

35 issues found. Ignoring 0 issues.

}
}

if !c.IsSet("addresstag") {

This comment has been minimized.

Copy link
@codelingo

codelingo bot Jan 6, 2020

The comments are uninterpreted plain text, so HTML and other
annotations such as this and will reproduce verbatim and should not be used.

Amount: whereHelperfloat64{field: "\"withdrawal_history\".\"amount\""},
Description: whereHelpernull_String{field: "\"withdrawal_history\".\"description\""},
WithdrawType: whereHelperint{field: "\"withdrawal_history\".\"withdraw_type\""},
CreatedAt: whereHelpertime_Time{field: "\"withdrawal_history\".\"created_at\""},

This comment has been minimized.

Copy link
@codelingo

codelingo bot Jan 6, 2020

Use MixedCaps or mixedCaps rather than underscores to write multiword names. From Effective Go.

Status: whereHelperstring{field: "\"withdrawal_history\".\"status\""},
Currency: whereHelperstring{field: "\"withdrawal_history\".\"currency\""},
Amount: whereHelperfloat64{field: "\"withdrawal_history\".\"amount\""},
Description: whereHelpernull_String{field: "\"withdrawal_history\".\"description\""},

This comment has been minimized.

Copy link
@codelingo

codelingo bot Jan 6, 2020

Use MixedCaps or mixedCaps rather than underscores to write multiword names. From Effective Go.

Description: whereHelpernull_String{field: "\"withdrawal_history\".\"description\""},
WithdrawType: whereHelperint{field: "\"withdrawal_history\".\"withdraw_type\""},
CreatedAt: whereHelpertime_Time{field: "\"withdrawal_history\".\"created_at\""},
UpdatedAt: whereHelpertime_Time{field: "\"withdrawal_history\".\"updated_at\""},

This comment has been minimized.

Copy link
@codelingo

codelingo bot Jan 6, 2020

Use MixedCaps or mixedCaps rather than underscores to write multiword names. From Effective Go.

Status whereHelperstring
Currency whereHelperstring
Amount whereHelperfloat64
Description whereHelpernull_String

This comment has been minimized.

Copy link
@codelingo

codelingo bot Jan 6, 2020

Use MixedCaps or mixedCaps rather than underscores to write multiword names. From Effective Go.

}
func (w whereHelpernull_String) IsNull() qm.QueryMod { return qmhelper.WhereIsNull(w.field) }
func (w whereHelpernull_String) IsNotNull() qm.QueryMod { return qmhelper.WhereIsNotNull(w.field) }
func (w whereHelpernull_String) LT(x null.String) qm.QueryMod {

This comment has been minimized.

Copy link
@codelingo

codelingo bot Jan 6, 2020

Use MixedCaps or mixedCaps rather than underscores to write multiword names. From Effective Go.

func (w whereHelpernull_String) GT(x null.String) qm.QueryMod {
return qmhelper.Where(w.field, qmhelper.GT, x)
}
func (w whereHelpernull_String) GTE(x null.String) qm.QueryMod {

This comment has been minimized.

Copy link
@codelingo

codelingo bot Jan 6, 2020

Use MixedCaps or mixedCaps rather than underscores to write multiword names. From Effective Go.

func (w whereHelpernull_String) LT(x null.String) qm.QueryMod {
return qmhelper.Where(w.field, qmhelper.LT, x)
}
func (w whereHelpernull_String) LTE(x null.String) qm.QueryMod {

This comment has been minimized.

Copy link
@codelingo

codelingo bot Jan 6, 2020

Use MixedCaps or mixedCaps rather than underscores to write multiword names. From Effective Go.

}{
ID: whereHelperint64{field: "\"withdrawal_crypto\".\"id\""},
Address: whereHelperstring{field: "\"withdrawal_crypto\".\"address\""},
AddressTag: whereHelpernull_String{field: "\"withdrawal_crypto\".\"address_tag\""},

This comment has been minimized.

Copy link
@codelingo

codelingo bot Jan 6, 2020

Use MixedCaps or mixedCaps rather than underscores to write multiword names. From Effective Go.

var WithdrawalCryptoWhere = struct {
ID whereHelperint64
Address whereHelperstring
AddressTag whereHelpernull_String

This comment has been minimized.

Copy link
@codelingo

codelingo bot Jan 6, 2020

Use MixedCaps or mixedCaps rather than underscores to write multiword names. From Effective Go.

@xtda xtda added review me nomerge and removed review me labels Jan 6, 2020
Copy link
Collaborator

gloriousCode left a comment

This is a nice fancy draft! 🎉 These are just a few observations and nothing srs

CreatedAt: time.Now().String(),
}
if req.RequestDetails.Description != "" {
tempEvent.Description.SetValid(req.RequestDetails.Description)

This comment has been minimized.

Copy link
@gloriousCode

gloriousCode Jan 7, 2020

Collaborator

This is a minor nit as you've done this for some of these already 👍 , but I would like this kind of validation to happen at an earlier point too. eg SubmitWithdrawal. While its necessary what you're doing here, we can stop code ever reaching this point if its invalid.

I'm not saying remove this though, since I think its needed to maintain database integrity, but it saves a lot of processing time dismissing bad data earlier

Additionally, WithdrawalHistory struct has omitempty which I figure would lead to a null insert. Does that not happen?

This comment has been minimized.

Copy link
@xtda

xtda Jan 7, 2020

Author Member

The validation here is purely to meet the requirements for database insertion (in this case null v not null)

I do have a validation when a withdrawal request is submitted https://github.com/xtda/gocryptotrader/blob/withdrawv2/withdraw/validate.go

that gets called as one of the first things in SubmitWithdrawal() https://github.com/xtda/gocryptotrader/blob/24ea80150631c80d43a3e498c8aced34cd3328ef/engine/withdraw.go#L17

Its still basic at the moment i am working on fleshing it out
Is this what you had in mind?

if c.Args().Get(2) != "" {
address = c.Args().Get(2)
}
}

if !c.IsSet("addresstag") {
if c.Args().Get(2) != "" {
addresstag = c.Args().Get(3)
}
}

if !c.IsSet("amount") {
if c.Args().Get(3) != "" {
amountStr, err := strconv.ParseFloat(c.Args().Get(4), 64)
if err == nil {
amount = amountStr
}
}
}

if !c.IsSet("fee") {
if c.Args().Get(3) != "" {
feeStr, err := strconv.ParseFloat(c.Args().Get(5), 64)
if err == nil {
fee = feeStr
}
}
}

if !c.IsSet("description") {
if c.Args().Get(2) != "" {
description = c.Args().Get(6)
Comment on lines 2075 to 2106

This comment has been minimized.

Copy link
@gloriousCode

gloriousCode Jan 7, 2020

Collaborator

Are these the correct numbers to be checking?
eg Looks like like retrieving the withdrawal amount is dependent on the addresstag being there, but addresstags are not required for all currencies

This comment has been minimized.

Copy link
@xtda

xtda Jan 7, 2020

Author Member

they are in correct good pickup

I am also on the fence about usage of unnamed params here (e.g just passing in order "address" "tag" "amount" "fee" instead of --address because i feel it could be error prone

This comment has been minimized.

Copy link
@thrasher-

thrasher- Jan 10, 2020

Collaborator

On these big arg ones it's very risky, but I think it's still nice to offer both (I personally always use keys). On the server side, our withdraw validation should pick up any issues in case the user bodges them and I see our validation checks only getting better as time goes

This comment has been minimized.

Copy link
@xtda

xtda Jan 16, 2020

Author Member

Didn't mean to ignore the replies here back working on this today 🎉

Yeah the main concern for me was the address being out of place that can cause issues as a lot of exchanges will send even if its not valid

But as @thrasher- pointed out i have improved and will continue to improve client/gct side validation to address these and make it as error-prone free as possible

}

if !c.IsSet("bankaccountid") {
if c.Args().Get(2) != "" {

This comment has been minimized.

Copy link
@gloriousCode

gloriousCode Jan 7, 2020

Collaborator

Same here. Looks like you can only get a bankaccountid if you've populated the description field.

@@ -0,0 +1,65 @@
package engine

This comment has been minimized.

Copy link
@gloriousCode

gloriousCode Jan 7, 2020

Collaborator

We seem to use plurals for filenames here. Eg your previous event table entries is named events and order management is under orders 🤷‍♂

}

id, _ := uuid.NewV4()
resp := &withdraw.Response{

This comment has been minimized.

Copy link
@gloriousCode

gloriousCode Jan 7, 2020

Collaborator

Its a response here but a request (req) in the function you pass it to. Seems like you could pass in the actual request, and return withdrawResponse and err

Right now, you can have an error in withdrawal.Event(resp) but the caller of SubmitWithdrawal has no idea that there is an error


var exchID string
if req.Type == withdraw.Crypto {
exchID, err = exch.WithdrawCryptocurrencyFunds(req)

This comment has been minimized.

Copy link
@gloriousCode

gloriousCode Jan 7, 2020

Collaborator

You're probably aware, but you'll need to review the implementations here before relying on these. eg Bithumb dismisses the response 🎉 🎉 🎉

This comment has been minimized.

Copy link
@xtda

xtda Jan 7, 2020

Author Member

Yep
I have been going through exchanges i have funds on to test and confirm but will be doing a full pass of all exchanges to test that withdraw params & response match up

id, _ := uuid.NewV4()
resp := &withdraw.Response{
ID: id,
ExchangeID: exchID,

This comment has been minimized.

Copy link
@gloriousCode

gloriousCode Jan 7, 2020

Collaborator

Is exchange ID meant to be the withdrawal id returned afterwards? I think withdrawalID would be a better fit

This comment has been minimized.

Copy link
@xtda

xtda Jan 7, 2020

Author Member

Yes it is the identifier the exchange returns happy to change it to withdrawalID makes more sense

@xtda xtda mentioned this pull request Jan 22, 2020
10 of 10 tasks complete
xtda added 3 commits Jan 22, 2020
… Clear
@xtda xtda mentioned this pull request Jan 22, 2020
11 of 11 tasks complete
xtda added 3 commits Jan 22, 2020
xtda added 18 commits Jan 23, 2020
WIP
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.