-
Notifications
You must be signed in to change notification settings - Fork 501
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
services/horizon/internal: Add fee_charged and max_fee to /fee_stats endpoint #1964
Conversation
ht.Assert.Equal(200, result.MaxFee.P95, "p95") | ||
ht.Assert.Equal(200, result.MaxFee.P99, "p99") | ||
|
||
ht.Assert.Equal(50, result.FeeCharged.Min, "min") |
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.
@bartekn can fee_charged be lower than 50? If not, we probably should make an update on the fee_charged
entries in this test scenario?
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.
Have you found an answer to the question? I think the code is outdated so can't check it out.
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.
@bartekn yes, after scrolling around I came to the conclusion that it's done based on the data we have in the DB, which is fine then.
oh, I just realized I forgot to add max, will do that first thing in the morning. |
a163964
to
1c6f06a
Compare
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 must fix type connected issues.
@@ -16,26 +17,35 @@ import ( | |||
|
|||
var _ actions.JSONer = (*FeeStatsAction)(nil) | |||
|
|||
// this struct is very similar to hProtocol.feeStats but drops the usage of int | |||
// in favor of int64 | |||
type feeStats 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.
this is a temporary workaround while we update hProtocol.FeeStats
@bartekn I addressed your comments, thanks! |
PR Checklist
PR Structure
otherwise).
services/friendbot
, orall
ordoc
if the changes are broad or impact manypackages.
Thoroughness
.md
files, etc... affected by this change). Take a look in the
docs
folder for a given service,like this one.
Release planning
needed with deprecations, added features, breaking changes, and DB schema changes.
semver, or if it's mainly a patch change. The PR is targeted at the next
release branch if it's not a patch change.
What
Display fees charged and max fee in /
fee_stats
endpoint. Fixes #1372Why
We have a way now to display not only
max_fee
but alsofee_charged
data on the/fee_stats
end-point. This PR adds info for both fields and at the same time schedules the old field for deprecation which had a confusing name as described #1372 (comment)Known limitations
[TODO or N/A]