-
Notifications
You must be signed in to change notification settings - Fork 25
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
Wallet Metric #768
Wallet Metric #768
Conversation
a2bc9cf
to
e9e7f5d
Compare
api/metrcis.go
Outdated
WalletMetric struct { | ||
Timestamp time.Time `json:"timestamp"` | ||
|
||
Address types.Address `json:"address"` |
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 think the address isn't very useful considering it never changes and is not something that you can visualise in a meaningful way.
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 was on the fence when I wrote it. I figured it couldn't hurt but removed it. It wasn't about ever needing to visualise multiple wallets or handle multiple addresses, it was more to ensure we never visualise anything we know not be related to "our wallet".
Unconfirmed types.Currency `json:"unconfirmed"` | ||
} | ||
|
||
WalletMetricsQueryOpts struct{} |
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.
kept this for consistency and extensibility
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 but could still use an integration test to verify that we register metrics
I actually added it and then removed it, I also tested it manually on my node. I'll add an integration test in a F/U PR. |
This PR adds a
WalletMetric
so we can easily fetch the evolution of the balance over time. This would prevent the wallet page from having to load all transactions (80mb) to accomplish the same. This would allow @alexfreska to fetch the latest transactions (usinglimit=100
) and create the balance graph using the metrics endpoint. This is consistent withhostd