Use hashed trading account for metrics user ID (part of #516) #534
Conversation
debnil
changed the title
cmd/trade.go
Outdated
| if botConfig.IsTradingSdex() { | ||
| userIDPrehash = botConfig.TradingAccount() | ||
| } else { | ||
| userIDPrehash = amplitudeAPIKey |
There was a problem hiding this comment.
this should use the hash of the API key for the centralized exchange.
By using the amplitudeAPIKey, all users on centralized exchanges will have the same userID, which is not what we want here.
cmd/trade.go
Outdated
| userIDPrehash = amplitudeAPIKey | ||
| } | ||
|
|
||
| userIDHashed, e := utils.HashString(userIDPrehash) |
There was a problem hiding this comment.
can you add a comment describing why we take the hash?
reason is so we don't expose the user's account or api_key.
cmd/trade.go
Outdated
| logger.Fatal(l, fmt.Errorf("could not create user id: %s", e)) | ||
| } | ||
|
|
||
| userID := fmt.Sprint(userIDHashed) |
There was a problem hiding this comment.
the newly added code is very self-contained. It may be cleaner if we put it in a separate function call, what do you think?
userID, e := getUserID(botConfig)
if e != nil {
logger.Fatal(...)
}
cmd/trade.go
Outdated
| // hash avoids exposing the user account or api key | ||
| userIDHashed, e := utils.HashString(userIDPrehash) | ||
| if e != nil { | ||
| logger.Fatal(l, fmt.Errorf("could not create user id: %s", e)) |
There was a problem hiding this comment.
wherever we can return an error we should. it's better than deciding how to handle the error at these inner function levels which is an unexpected "side effect" of the function.
cmd/trade.go
Outdated
| } | ||
|
|
||
| userID := fmt.Sprint(userIDHashed) | ||
| return userID, e |
There was a problem hiding this comment.
error will always be nil here. can return fmt.Sprint(userIDHashed), nil
we do this as a convention across the codebase which gives the reader of the code confidence in knowing that all errors have been handled and bubbled up appropriately as soon as they were encountered. It makes the code a little easier to reason about as the codebase gets bigger
This PR uses the hash of the bot's trading account as the user ID for metrics collection. If the user is using a centralized exchange, then it uses the hash of the Amplitude API key. Part of #516.
After: We see a different integer as the User ID from those previously used, on a build from this branch.
