Feat/binance exchange ws get trade history/get latest trade cursor #719

tibrn · 2021-07-19T17:24:51Z

Guidelines for submitting code to Kelp

Before Submitting a PR

Problem Identification
1. ensure you understand the problem / task.
2. ask initial questions to understand the task described in the Github issue, which may not be clearly defined.
3. you can reach the team on keybase or by commenting on the Github issue.
4. this is not the point to discuss concrete solutions (concrete design or implementation), but only what the problem is and the desired outcome.
5. ask any questions related to scope of work, breakdown of tasks into subtasks and PRs.
6. typically this should be covered in the Github issue. If it is not, then add to the issue after discussion with code owner.
7. author needs to be comfortable with breakdown and scope of tasks. Scope should be small. Think about the overhead on the code reviewer when breaking down the tasks.
Research Solution
1. conduct research on the issue at hand. Identify point of code change and any “fallout” from the change (tip: search the codebase for consumers of the function / object, and their consumers etc., until you get to a part of the code that you are well-versed with).
2. author needs to become the “expert” on the part of the code being touched before submitting a code change.
3. If it will take you 2 more days to figure something out by reading the code or experimenting with the codebase then do it.
  1. This is not wasted effort.
  2. It is likely that the code reviewer would use a similar process to figure it out, but this is your job and not the code reviewer’s job.
4. If the process of becoming an “expert” on a specific area would take you more than 2 days then discuss this with the code reviewer if it is worth it, and proceed accordingly.
5. Think about what kind of tests you would want. Are you going with a black-box testing approach, or a white-box testing approach? Why?
Technical Design Discussion
1. PRs are not the best medium to conduct design discussions. The team is readily accessible and available so 1-1 meetings can be very effective.
2. This is the best avenue to discuss your ideas for how to approach the problem.
3. Prepare any relevant technical documentation for the design discussion and post on the Github issue and ping code owner.
  1. This can be in the form of a google document, flowchart diagram, or in rare cases a code snippet in a Draft PR.
  2. The Draft PR should not be reviewed by the code reviewer as part of the code change but can be used as a space to get feedback
  3. Resulting comments on a Draft PR can capture any guidance from the technical discussion.
  4. All Draft PRs should be discarded and never converted to a non-draft PR to avoid the overhead of comments lingering from the design discussion using the Draft PR.
4. Schedule technical design discussion to go over the proposed solution.
5. Include a list of tests you need to add, remove, modify. Consider all possibilities (tip: data related tests would require many test cases).
Implementation
1. implement feature or bug fix. This should be the first line of code that you write for the PR.
2. Avoid for loops to “autogenerate” test cases, unless absolutely necessary.
Pull Request
1. The first version of the PR should not have any “surprises” compared to what was discussed in the technical design.
2. See Handling of PRs section below for guide on how to get a PR to merged status.
3. See detailed guide on submitting code-reviewer-friendly PRs. This guide will save a lot of time for both author and reviewer and will help speed up the turnaround time on PRs.

Handling of PRs

Typically, this is what is expected from the author of a Pull Request:

Each PR should correspond to the smallest independent logical unit of change.
1. each PR should be prefixed with "bugfix", "feature", or "refactor".
2. refactors should never be mixed in with logic changes or bug fixes and vice-versa.
3. refactors that change variable names across more than 1 function should be a separate PR altogether.
Author should add inline commentary on the code changes to "guide" the reviewer on how to read the code change, highlighting key changes made.
1. this should typically be 2-3 places, as anything bigger is too big of a PR and should be split up.
We should aim for the following benchmarks in terms of PR efficiency:
1. In 60% of the cases the PR should be approved without any changes requested from the reviewer.
2. In 25% of the cases the reviewer should add 2-3 comments that need to be "fixed up". This is most likely a code style suggestion.
3. In 10% of the cases the reviewer should request a structural change to the code. These should not be "new ideas".
4. In <=5% of the cases the PR may be discarded because we have learned something new in the process of implementation.
Author makes requested changes.
1. no "new ideas" to be introduced at this stage.
2. no structural changes to be introduced beyond what is requested.
3. focus should be on minimizing "changes" beyond what is requested by reviewer.
4. person who created a comment / raised a question on the PR is responsible to mark it as resolved once they are satisfied. The developer should not mark questions raised by the reviewer as resolved since the reviewer uses these comments as a way to track what they are looking for in the next iteration of the review (tip: use an emoji such as the checkbox emoji to mark an inline comment as handled or addressed).
Reviewer re-reviews the code.
1. result of the PR should be that the PR has moved to the next stage in the review ladder described above.
2. this means that a PR will take a maximum of 2 review rounds to get to completion.
3. back to the previous step for the author to make changes based on the re-review.

Draft PRs

If there is a need to "try out the change" and get feedback, we can use a draft PR, although this should be used sparingly.
If a PR starts off in draft state then it should be used more as a "whiteboard for discussion" as opposed to the early stage of a PR.
In most cases, Draft PRs should be discarded. To avoid noise from comments, Draft PRs should be resubmitted as fresh standalone PRs instead of converting the draft PR to a regular PR.

nikhilsaraf

added some comments. this one turned out to be a bit heavier than the first two.

nikhilsaraf · 2021-07-19T20:32:00Z

plugins/binanceExchange_ws.go

+	Name string `json:"e"`
+}
+
+// "E": 1499405658658,            // Event time


can you please put these comments next to the struct, will be much easier to read and more meaningful there.

PS: Is this struct copied from somewhere where we can import it rather than defining it ourselves?

PPS: if it's not defined elsewhere but it captures the return values of the binance web socket API then it's probably better suited to live in this new file /support/sdk/binance-ws.go

nikhilsaraf · 2021-07-19T20:39:32Z

plugins/binanceExchange_ws.go

+	})
+
+	return &stream{doneC: doneC, stopC: stopC, cleanup: func() {
+


if there's nothing defined in the cleanup function then we can pass nil?
(or was the intention to pass in some cleanup code here?)

nikhilsaraf · 2021-07-19T20:41:06Z

plugins/binanceExchange_ws.go

@@ -220,6 +394,19 @@ func subcribeBook(symbol string, state *mapEvents) (*stream, error) {

 }

+//ListenKey expires every 60 minutes
+func keepAliveStreamService(client *binance.Client, key string) {


is this the function that keeps pinging binance so it maintains the connection?

nikhilsaraf · 2021-07-19T20:43:29Z

plugins/binanceExchange_ws.go

+		log.Printf("Error keepAliveStreamService %v\n", err)
+	}
+
+	go keepAliveStreamService(client, key)


why run this in a new goroutine?

We should run this without kicking off a new goroutine and the code that calls keepAliveStreamService for the first time should kick off the goroutine.

the way it works right now (if I'm reading correctly, not sure), then when you call makeBinanceWs it will sleep the main thread for 50 minutes. If that's true then that may not be what we are looking for and the above suggestion may help improve the situation

nikhilsaraf · 2021-07-19T20:45:16Z

plugins/binanceExchange_ws.go

+	listenKey, err := binanceClient.NewStartUserStreamService().Do(context.Background())
+
+	if err != nil {
+		return nil, err


can you wrap the error like:

fmt.Errorf("some meaningful message like 'unable to start new user stream service': %s", err)

wrapping errors provide a little more context and a better stack trace.

nikhilsaraf · 2021-07-19T21:14:37Z

plugins/binanceExchange_ws.go

+		err := json.Unmarshal(message, event)
+
+		if err != nil {
+			log.Printf("Error unmarshal %s to eventExecutionReport\n", string(message))


If this fails here then does that mean that subcribeUserStream has failed?

If so then we should also cause subcribeUserStream to return an error in this case.

the only way to capture errors when binance it's sending events it's to use another map(state) to save the error when happens and when any method from the interface it's called to check if for the stream "X" which it's using is any error

If the stream errors in update 1, but passes in update 2, and the user fetched the value of update 2 only, we don’t care about the error in update 1.

With that in mind, what I’m thinking is that when we write a value to the existing map (the data structure), we also include an error. So if there is an error in the stream we should write an error value.

When the user fetched the value they will check the error first and if it is an error they will handle appropriately (bubble upstream IIRC).

That allows us to keep the date of the response and error encapsulated into one map. What do you think?

nikhilsaraf · 2021-07-19T21:15:02Z

plugins/binanceExchange_ws.go

+			return
+		}
+
+		userStreamLock.Lock()


why do we need to lock here?
wondering in which scenario may this be executed twice, or maybe I've missed something

this is to avoid any misusage of the callback and to not have invalid data, because I am modifying the slice in the callback and the possibility for that callback to be executed by two goroutines exists I've added the mutex

Ok will look at this a little more closely when I’m on a computer

nikhilsaraf · 2021-07-19T21:18:22Z

plugins/binanceExchange_ws.go

+		data, isOk := history.data.(History)
+
+		if !isOk {
+			log.Printf("Error conversion %v\n", ErrConversionHistory)


should this also cause the stream to fail and throw an error somewhere?

I'm concerned that all these errors here will be hidden errors that will not cause the code to crash.

we want the bot to crash rather than throw silent errors

one option it's to panic when I get errors from the stream, the second it's the one that I've written above

See above comment. Once we agree on a pattern we can apply to all streams.

nikhilsaraf · 2021-07-19T21:24:07Z

plugins/binanceExchange_ws.go

+					lastCursor = cursor
+				}
+			} else {
+				log.Printf("Error converting cursor %v\n", ErrConversionCursor)


maybe we should return an error here rather than just logging. we want the bot to crash if there is any error

nikhilsaraf · 2021-07-19T21:27:00Z

plugins/binanceExchange_ws.go

@@ -140,12 +220,14 @@ func makeMapEvents() *mapEvents {
 type events struct {
 	SymbolStats *mapEvents
 	BookStats   *mapEvents
+	OrderEvents *mapEvents


Why is this called OrderEvents?

I'm confused because we are fetching the User Stream and getting Trade History, so was expecting this to be called either TradeHistoryEvents or UserEvents

nikhilsaraf · 2021-07-25T20:24:00Z

plugins/binanceExchange_ws.go

+
+		if err != nil {
+			log.Printf("Error keepAliveStreamService %v\n", err)
+			panic(err)


We should find a way to not panic here. Sorry didn’t see this earlier.
Panics are very bad in kelp for many reasons and I can go into detail if interested.

tibrn added 10 commits July 18, 2021 16:35

add GetOrderBook

44de487

add more comments

2dfbd2d

move comment on next line

1a5cfb3

patch/resolve comments

af75622

patch/ fixes

9518463

patch/ increase waitTime

f4e975a

add timeout instead of waiting

a35faa3

patch/ remove timeout

a75195e

patch/ check for len > fetchSize

2616502

add GetTrageHistory + GetLatestTradeCursor

df8c006

tibrn requested a review from nikhilsaraf as a code owner July 19, 2021 17:24

tibrn added 2 commits July 19, 2021 20:27

patch/ resolve conflicts

ac093cb

avoid testing when short flag is set

0c41ff3

nikhilsaraf reviewed Jul 19, 2021

View reviewed changes

tibrn added 2 commits July 25, 2021 21:04

refactor

17a422b

wrap error for creating WsUserDataService

65443c6

nikhilsaraf reviewed Jul 25, 2021

View reviewed changes

tibrn added 3 commits July 26, 2021 21:21

handle errors + refactor

91837ec

wrap errors

2f5963a

rename error

88d2b24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/binance exchange ws get trade history/get latest trade cursor #719

Feat/binance exchange ws get trade history/get latest trade cursor #719

tibrn commented Jul 19, 2021

nikhilsaraf left a comment

nikhilsaraf Jul 19, 2021

nikhilsaraf Jul 19, 2021

nikhilsaraf Jul 19, 2021

nikhilsaraf Jul 19, 2021

nikhilsaraf Jul 19, 2021

tibrn Jul 25, 2021

nikhilsaraf Jul 19, 2021

nikhilsaraf Jul 19, 2021

nikhilsaraf Jul 19, 2021

tibrn Jul 25, 2021

nikhilsaraf Jul 25, 2021

nikhilsaraf Jul 19, 2021

tibrn Jul 25, 2021

nikhilsaraf Jul 25, 2021

nikhilsaraf Jul 19, 2021

tibrn Jul 25, 2021 •

edited

Loading

nikhilsaraf Jul 25, 2021

nikhilsaraf Jul 19, 2021

nikhilsaraf Jul 19, 2021

nikhilsaraf Jul 25, 2021

		})

		return &stream{doneC: doneC, stopC: stopC, cleanup: func() {

Feat/binance exchange ws get trade history/get latest trade cursor #719

Are you sure you want to change the base?

Feat/binance exchange ws get trade history/get latest trade cursor #719

Conversation

tibrn commented Jul 19, 2021

Guidelines for submitting code to Kelp

Before Submitting a PR

Handling of PRs

Draft PRs

nikhilsaraf 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

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

tibrn Jul 25, 2021 • edited Loading

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

tibrn Jul 25, 2021 •

edited

Loading