-
Notifications
You must be signed in to change notification settings - Fork 3
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
Waggle sign API server #11
Conversation
Resolves #11 |
@bugout-dev check
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like availableSigners
global variable. Left a few other comments and suggestions.
func ReadServerConfig(rawConfigPath string) (*[]ServerSignerConfig, error) { | ||
var configs []ServerSignerConfig | ||
|
||
configPath := strings.TrimSuffix(rawConfigPath, "/") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, if path will be passed acidently with trailing slash, like --config ~/config.json/
if err != nil { | ||
log.Fatal(err) | ||
} | ||
configTemp := &[]ServerSignerConfig{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to use:
configTemp := []ServerSignerConfig{}
unmarshalErr := json.Unmarshal(rawBytes, &configTemp)
This is the pattern we use everywhere else:
$ git grep Unmarshal
bugout.go: parseErr := json.Unmarshal([]byte(result.Content), &reports[i])
cmd.go: parseErr := json.Unmarshal(batchRaw, &batch)
cmd.go: rowParseErr := json.Unmarshal(jsonString, &batch[i])
cmd.go: parseErr := json.Unmarshal(messagesRaw, &messages)
moonstream.go: unmarshalErr := json.Unmarshal(responseBody, &contracts)
moonstream.go: unmarshalErr := json.Unmarshal(responseBody, &callRequests)
server.go: err = json.Unmarshal(body, &req)
settings.go: err = json.Unmarshal(rawBytes, configTemp)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two things:
- Reference passed in
json.Unmarshal
call rather than declaring variable to be the reference. - Create a new error for unmarshalling to make the code more readable -
unmarshalErr
instead oferr
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used this, because nodebalancer, monitoring, etc code written in this way. If it is important, I can follow as you wish.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I am using err
because:
func abc(someInts []*int) (int, error) {
var num *int
for _, si := range someInts {
// Here I forced to use `:=` but I want to be asure to use variable `num` scope from func `abc`
num, numErr := someOperation(si)
...
}
}
So I prefer this code:
func abc(someInts []*int) (int, error) {
var num *int
var err error
for _, si := range someInts {
num, err = someOperation(si)
...
}
}
server.go
Outdated
json.NewEncoder(w).Encode(req) | ||
} | ||
|
||
func ServerRun(host string, port int) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we rename it simpler? func Serve(...) error {}
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me its is not clear naming, but ok
server.go
Outdated
) | ||
|
||
var ( | ||
availableSigners map[string]AvailableSigner |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the only big problem I have with this PR. Why does this need to be a global variable?
Can't you pass it as an argument to the server?
This should definitely be an argument to ServerRun(host string, port int, availableSigners map[string]AvailableSigner)
.
You can create a WaggleServer
struct and define the handlers on that struct if you want, so that they have access to the availableSigners
member.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should avoid these kind of global variables. It limits how waggle
can be used (e.g. you can't instantiate multiple waggle servers in the same process)`.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Problem not in passing to ServerRun
but in passing variable in each mux
serveMux := http.NewServeMux()
serveMux.HandleFunc("/ping", pingRoute)
serveMux.HandleFunc("/sign/dropper", signDropperRoute)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure how I ll pass in signDropperRoute
@@ -504,3 +565,52 @@ func CreateMoonstreamCommand() *cobra.Command { | |||
|
|||
return moonstreamCommand | |||
} | |||
|
|||
func CreateServerCommand() *cobra.Command { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe better to have:
waggle server configure
(replacingwaggle accounts config
)waggle server run
This allows us to scope available signers on a per-server basis. Makes `waggle` more usable as a library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Handle API requests for batch sign
Example of API request:
Example of response: