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

Basic service impl #24

Merged
merged 2 commits into from Mar 13, 2019
Merged

Basic service impl #24

merged 2 commits into from Mar 13, 2019

Conversation

@moshababo
Copy link
Collaborator

@moshababo moshababo commented Mar 12, 2019

Continuing previous commit.
This is WIP, but need to be merged before the following gRPC PR.

Documentation & tests will be added after API will get stabilized.

@moshababo moshababo requested a review from antonlerner Mar 12, 2019
service/service_test.go Outdated Show resolved Hide resolved
res.openRoundId = s.openRound.id

ids := make([]int, 0, len(s.executingRounds))
for id := range s.executingRounds {
Copy link
Member

@antonlerner antonlerner Mar 12, 2019

must be interlocked with NewService

Copy link
Collaborator Author

@moshababo moshababo Mar 12, 2019

Sorry, I didn't understand what you mean.

Copy link
Member

@antonlerner antonlerner Mar 12, 2019

executingRounds is accessed here and in another go routine, please use a mutex when reading from same memory concurrently.

Copy link
Collaborator Author

@moshababo moshababo Mar 13, 2019

Yes, i'll left a TODO for this in the other goroutine. I'll use the same mutex which will be applied there.

err := r.submitCommitment(c)
if err != nil {
return nil, err
}

res := new(submitResponse)
res := new(SubmitCommitmentResponse)
Copy link
Member

@antonlerner antonlerner Mar 12, 2019

no actual need for new, you can also refactor the two lines to ine using res := SubmitCommitmentResponse {roundId : r.id}

Copy link
Collaborator Author

@moshababo moshababo Mar 12, 2019

Do you mean &SubmitCommitmentResponse {roundId : r.id}?
If so, since they are equivalent, you mean that that’s the preferred style?
I'm asking because I used new in all other pointer constructs as well.

Copy link
Member

@antonlerner antonlerner Mar 12, 2019

Yes, using new keyword is not common in go apps, but it is not important to refactor it. Just wanted to mention the alternative that is also more compact

@moshababo moshababo merged commit bda5d8a into develop Mar 13, 2019
@moshababo moshababo deleted the service branch Mar 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants