Skip to content
This repository has been archived by the owner on Feb 1, 2024. It is now read-only.

Add isTestnet to metric common props (part of #516) #530

Merged
merged 2 commits into from
Oct 13, 2020

Conversation

debnil
Copy link
Contributor

@debnil debnil commented Oct 13, 2020

This PR adds an isTestnet boolean field to the commonProps struct used in tracking metrics. Part of #516.

Below screenshot verifies that if the userID is set to "12345" and tested locally, an event with the isTestnet property is sent.
Verification of isTestnet property

@debnil debnil requested a review from nikhilsaraf as a code owner October 13, 2020 18:07
@debnil debnil added this to the v1.10.0 milestone Oct 13, 2020
Copy link
Contributor

@nikhilsaraf nikhilsaraf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one comment, rest looks good.

cmd/trade.go Outdated
@@ -527,6 +527,7 @@ func runTradeCmd(options inputs) {
botConfig.TickIntervalSeconds,
botConfig.TradingExchange,
botConfig.TradingPair(),
strings.Contains(botConfig.HorizonURL, "test"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be false if trading on centralized exchanges

Copy link
Contributor

@nikhilsaraf nikhilsaraf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good 🚢
thanks for keeping these reviews small, they're much simpler to review.

when you commit these, if there's a way to make the commits show up in the GH issue (probably by referencing the issue number with a hash in the commit message) that would be great for future reference :)

@debnil debnil merged commit 98b31b2 into master Oct 13, 2020
@debnil debnil deleted the debnil/cli-props-testnet branch October 13, 2020 21:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants