-
Notifications
You must be signed in to change notification settings - Fork 48
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
Add user agent and initiator string to account open and order submission calls #211
Conversation
The Makefile contained a number of copy/paste errors that we fix with this commit: The make_ldflags function and RELEASE_TAGS variable were missing and therefore resulted in empty strings. With those things removed, we can then go ahead and build the correct LDFLAGS variable and actually use it when building or installing the binaries.
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.
Cool approach 😀
var result bytes.Buffer | ||
for _, r := range str { | ||
if strings.ContainsRune(semanticAlphabet, r) { | ||
if strings.ContainsRune(alphabet, r) { |
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.
isn't it better to just return an error if this is not true?
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.
Perhaps, yes. But that'd require us to thread through the error in a few places and bug the user with an error for something that's just a nice to have (in case of the initiator). Or do you mean for the semantic version as well?
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.
Was thinking just in general. If an invalid user agent or semantic is set, we will fail to start. But perhaps I'm missing some case this would be disruptive for.
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.
Okay, I added a check for the version and agent name on startup and also when overwriting the agent name.
I think we really don't care enough about the initiator being silently truncated for an error to be threaded through so I didn't change that behavior.
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 take it back, better to do what you had initially :)
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.
Okay, I reverted the change with the init()
function but left the check in for when the agent name is overwritten externally.
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 🌮
Added a version bump commit as well so we can include a tagged version in LiT. |
var result bytes.Buffer | ||
for _, r := range str { | ||
if strings.ContainsRune(semanticAlphabet, r) { | ||
if strings.ContainsRune(alphabet, r) { |
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 take it back, better to do what you had initially :)
// SetAgentName sets the static part of the user agent string, identifying the | ||
// type of binary that is running. This function panics if the agent name | ||
// contains characters outside of the allowed semantic alphabet. | ||
func SetAgentName(newAgentName string) { |
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 used?
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, it'll be used in LiT to overwrite the default name. Updated the comment to make it clearer.
As a preparation to normalize the initiator part of the user agent, we allow specifying the alphabet for the normalizeVerString function.
To be able to identify the type of Pool daemon binary version that submits orders and accounts, we want to send a user agent string to the server. That string can be customized when poold is integrated into LiT for example.
To make it easy to pass the initiator of an action to the auctioneer client, we use the incoming context to transport it. We provide two helper functions in the main package to wrap and unwrap the initiator string in a context.
To allow the server to gather some statistics about what software is running on the other end, we add a user agent string to the following calls: - InitAccount: to count what agent was used to create/open an account. - SubmitOrder: to count what agent submitted an order.
To give any CLI or UI the option to identify themselves, an initiator field is added that will be appended to the binary's user agent string before sending it to the server.
Send the static user agent plus the dynamic initiator string (if provided) to the auctioneer in the InitAccount and SubmitOrder RPCs.
In case the RPC client specified the initiator string we add it to the context of the InitAccount and SubmitOrder calls so it can be appended to the static user agent of the binary.
We add the initiator string "pool-cli" to the account open and order submission calls to give the server an idea what user facing software was used for those actions.
With the upcoming Pool UI we want to be able to see very rough statistics on what software is used for opening accounts and submitting orders. This will currently only distinguish between the standalone Pool daemon (
poold
) and the LiT integrated daemon (litd
) for the static user agent and then add the initiatorpool-cli
for calls issued with thepool
binary.