Implement the ability to provide a query with no filtering #7

p0mvn · 2021-10-22T05:22:00Z

I have a use case where I need to get all actions without any filters applied. The current design does not allow for that. As a result, for all API functions that would take queries as parameters, I refactored out a query interface that has specific implementation for each API under query package. This is a breaking change.

allow for no filter passed by implementing a Query interface
refactor to reduce code duplication
add gomega module to use its matchers in unit tests
unit test new functionality
all unit tests pass
minimize duplication in newton.go
unit test Do
implement CancelOrder (missing from API)

In the future, I'd like to suggest to incorporate a ginkgo BDD testing framework. It works well with gomega and really does simplify writing unit tests. We can also create some CI jobs to auto run unit tests that do not require Newton client and secret key.

@dhiaayachi please let me know what you think

… omit certain parameters

… methods

dhiaayachi · 2021-10-22T12:57:11Z

Thank you for this @akhtariev. I will spend some time during the weekend on this.

About the BDD tests give it a try and let's see how it look.
👍 on the CI job for public API, I guess we can introduce a flag that filter out non public tests

dhiaayachi

Thank you for adding this @akhtariev , this is really good. I think we should get this further by having a generic Do func at the newton package level and transfer all the specifics to the query package (at that point we will rename it Request).
That said, I'm keen to accept this as is as a first step (with some comments I added fixed) as it a good step to the right direction.

Thank you again!!

newton.go

p0mvn · 2021-10-24T23:12:05Z

Thanks for reviewing @dhiaayachi ! Will address the comments in the next couple of days

dhiaayachi · 2021-11-12T22:09:16Z

👋 @akhtariev, are you still interested in pursuing this PR? I can do another pass of reviews whenever it's ready.

p0mvn · 2021-11-12T23:21:21Z

Hi @dhiaayachi ! Yes, I'll finish it up in the next couple of days. Sorry I've been busy lately

dhiaayachi · 2021-11-12T23:39:23Z

No worries. Take your time and ping me when you have something ready for a review.

Thank you for contributing !!

…on only when a specific environment variable is set

p0mvn · 2021-11-14T20:13:42Z

Hi @dhiaayachi ! Hope your weekend is going well. I just finished addressing your comments. I ended up refactoring it ti make it look similar to httpClient Do right away. I also implemented CancelOrder endpoint.

In addition, I noticed that Newton now has a mock server so I switched every unit test to query that mock server. However, the mock server does not test authentication so I added another test for that that must be enabled by setting TEST_AUTH environment variable. I added more info on that in the README.

Going forward, we can automate unit tests with GitHub Actions. Also, since the mock server simply hardcodes data in response, we can make more assertions on the response in the unit tests. Just to ensure that every response has the fields we expect it to have. However, we should probably do that in separate PRs.

Let me know what you think

dhiaayachi

Thank you for this great contribution @akhtariev. This is really good!! My weekend was good and you made it even better with this contribution 😃.
I made a few comments, but nothing really big, this is now really close. Can you please also gofmt the code, I'm planing on adding a linting step into the automated CI.

dhiaayachi · 2021-11-15T02:09:07Z

newton.go


-	var r ActionsResp
-	err = json.Unmarshal(body, &r.Actions)
+	parsedResponse, err := n.parseResponse(res, query.GetResponse())


This is for future improvements, but I think that we can make GetRepsonse return a defined interface, that wrap map[string]interface{} and have methods that get fields to the enduser, like GetAsString or GetAsFloat...

Created issue

I need to think about this a bit more. My intention is that the user don't have to cast response to the underlying type.
I think a better idea would be to make the user pass the response as a pointer and we populate it and that would be trivial. But let's do it out of this PR.

dhiaayachi · 2021-11-15T02:12:01Z

newton.go

+	if toParseTo == nil {
+		return parsedResponse, nil
 	}

-	body, _ := ioutil.ReadAll(res.Body)
+	body, err := ioutil.ReadAll(res.Body)


I think we should read the body first and then decide if we return a nil value and error if a body is returned and toParseTo is nil, the reason is that if the body of a request is not read properly by the client it can reset the connexion and the body is never properly freed.

Could you elaborate on this please? I'd assume it is safe since we close response's Body in defer. Why do we need to read the body in addition to closing to ensure that it is properly freed?

there is a special behaviour in go http client that if you close the body without reading the buffer it will reset the underlying connexion and that could be a source of performance issues. more details in here

thanks for the explanation

query/actions.go

export_test.go

dhiaayachi · 2021-11-15T02:24:32Z

auth_test.sh

@@ -0,0 +1,3 @@
+#!/bin/bash
+
+TEST_AUTH=true CLIENT_ID=$1 CLIENT_SECRET=$2 go test


This is ok for the time being but I think I can make even the auth test run in automated environment, I will start a PR for this as soon as this is merged

Sounds great. Would be curious to see how you make it work

I have some ideas, I will let you know the details if my experiment succeed 😃

…newton_test.go

p0mvn · 2021-11-15T08:20:25Z

@dhiaayachi Thanks for another round of review! I addressed all comments. However, the reasoning behind this one isn't exactly clear. Would you mind elaborating on that please just for me to learn

dhiaayachi

Great work @akhtariev !! 🎉
I will work on the CI as soon as I can, also the small change about the Do func signature. After that I will crank the release version to 0.10, as this is a fairly big API change.

akhtariev added 3 commits October 21, 2021 20:56

refactor private endpoints to extract a query interface to be able to…

ba00109

… omit certain parameters

implement quert for public api and pass query inteface to all private…

7e3ea23

… methods

refactor to reduce code duplication

6606f34

dhiaayachi reviewed Oct 24, 2021

View reviewed changes

newton.go Outdated Show resolved Hide resolved

newton.go Outdated Show resolved Hide resolved

newton.go Outdated Show resolved Hide resolved

newton.go Outdated Show resolved Hide resolved

newton.go Outdated Show resolved Hide resolved

newton.go Outdated Show resolved Hide resolved

akhtariev added 12 commits October 24, 2021 20:48

fix warnings about printing errors

c7ecc98

correct endpoints for OpenOrders and HealthCheck

9391f4d

remove "Get" prefixes from the API for consistency

916a89c

implement TickSizes query

d525d4e

rename query get parameter unit tests

788fcae

implement maximum trade amounts query

a50216a

implement applicable fees query

77638ad

implement health check query

b65703e

implement minimum trade amount query

554978e

implement query get body

00fd5fb

reafctor query GetBody to return string

62ed89c

pass query to Newton's Do

bb7d03e

p0mvn changed the title ~~Implement the ability to provide a query with no filtering~~ Draft: Implement the ability to provide a query with no filtering Oct 25, 2021

p0mvn marked this pull request as draft October 25, 2021 06:37

akhtariev added 7 commits October 24, 2021 23:48

implement and test GetPath and GetMethod for query

ee5e716

implement GetResponse for TickSizes

90dbd72

implement max and min trade amounts GetResponse

839ee76

test GetResponse for TickSizes, max and min trade amounts

0427615

implement and test GetResponse for all queries

7a1eb28

refactor main package and unit test

416d083

refactor newton's Do to return Response struct

5b2e98c

akhtariev added 4 commits November 14, 2021 11:23

test new order endpoint, implement mock testing and test authenticati…

33750fc

…on only when a specific environment variable is set

update README

4ea0115

create query for cancel order

2fd7e97

unit test cancel order

36ca84e

dhiaayachi reviewed Nov 15, 2021

View reviewed changes

gofmt

b5accbd

p0mvn mentioned this pull request Nov 15, 2021

Make GetResponse in query package return a defined interface #8

Open

akhtariev added 2 commits November 15, 2021 00:10

remove export_test from root package and duplicate proudction URL in …

8716e20

…newton_test.go

read body before any error handling when parsing response

31d2e93

p0mvn marked this pull request as ready for review November 15, 2021 08:20

p0mvn changed the title ~~Draft: Implement the ability to provide a query with no filtering~~ Implement the ability to provide a query with no filtering Nov 15, 2021

dhiaayachi approved these changes Nov 15, 2021

View reviewed changes

dhiaayachi merged commit 24823e5 into dhiaayachi:main Nov 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement the ability to provide a query with no filtering #7

Implement the ability to provide a query with no filtering #7

p0mvn commented Oct 22, 2021 •

edited

Loading

dhiaayachi commented Oct 22, 2021

dhiaayachi left a comment

p0mvn commented Oct 24, 2021

dhiaayachi commented Nov 12, 2021

p0mvn commented Nov 12, 2021

dhiaayachi commented Nov 12, 2021

p0mvn commented Nov 14, 2021

dhiaayachi left a comment

dhiaayachi Nov 15, 2021

p0mvn Nov 15, 2021

dhiaayachi Nov 15, 2021

dhiaayachi Nov 15, 2021

p0mvn Nov 15, 2021

dhiaayachi Nov 15, 2021

p0mvn Nov 16, 2021

dhiaayachi Nov 15, 2021

p0mvn Nov 15, 2021

dhiaayachi Nov 15, 2021

p0mvn commented Nov 15, 2021

dhiaayachi left a comment

		@@ -0,0 +1,3 @@
		#!/bin/bash

		TEST_AUTH=true CLIENT_ID=$1 CLIENT_SECRET=$2 go test

Implement the ability to provide a query with no filtering #7

Implement the ability to provide a query with no filtering #7

Conversation

p0mvn commented Oct 22, 2021 • edited Loading

dhiaayachi commented Oct 22, 2021

dhiaayachi left a comment

Choose a reason for hiding this comment

p0mvn commented Oct 24, 2021

dhiaayachi commented Nov 12, 2021

p0mvn commented Nov 12, 2021

dhiaayachi commented Nov 12, 2021

p0mvn commented Nov 14, 2021

dhiaayachi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p0mvn commented Nov 15, 2021

dhiaayachi left a comment

Choose a reason for hiding this comment

p0mvn commented Oct 22, 2021 •

edited

Loading