Skip to content
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 getFeeStats method #172

Merged
merged 20 commits into from
May 17, 2024
Merged

Add getFeeStats method #172

merged 20 commits into from
May 17, 2024

Conversation

2opremio
Copy link
Contributor

@2opremio 2opremio commented May 10, 2024

What

Add getFeeStats method, with the following response data structure:

type GetFeeStatsResult struct {
	SorobanInclusionFee FeeDistribution `json:"sorobanInclusionFee"`
	InclusionFee        FeeDistribution `json:"inclusionFee"`
	LatestLedger        uint32          `json:"latestLedger"`
}

type FeeDistribution struct {
	Max              uint64 `json:"max,string"`
	Min              uint64 `json:"min,string"`
	Mode             uint64 `json:"mode,string"`
	P10              uint64 `json:"p10,string"`
	P20              uint64 `json:"p20,string"`
	P30              uint64 `json:"p30,string"`
	P40              uint64 `json:"p40,string"`
	P50              uint64 `json:"p50,string"`
	P60              uint64 `json:"p60,string"`
	P70              uint64 `json:"p70,string"`
	P80              uint64 `json:"p80,string"`
	P90              uint64 `json:"p90,string"`
	P95              uint64 `json:"p95,string"`
	P99              uint64 `json:"p99,string"`
	TransactionCount uint32 `json:"transactionCount,string"`
	LedgerCount      uint32 `json:"ledgerCount"`
}

It includes extra fields with respect to #114 (a global latestLedger fieldand a count` field in each distribution, it indicates how many fees were included to compute it).

We can, optionally also include the initial ledger used for each distribution if that's interesting (which, together with the global latestLedger field, will tell the user what ledger window was used for each distribution).

Why

Closes #114

Known limitations

N/A

@2opremio
Copy link
Contributor Author

2opremio commented May 10, 2024

This is still untested but created early to gather feedback.

@tamirms I would like to know your opinion on the implementation. Unlike Horizon (which uses Postgres to compute the stats :S), we do everything in Go and book-keep an in-memory data structure. The juicy part is feewindow.go

@janewang Thoughts about the API?

@tamirms
Copy link
Contributor

tamirms commented May 10, 2024

@2opremio in-memory approach makes sense to me especially considering we sample at most 50 ledgers

@janewang
Copy link
Collaborator

I like the idea of fromLedger and latestLedger. I think it is helpful and more transparent on the fee calculation for the end user.

For count is a great idea but I think you mean the sample size for the statistic distribution. I think maybe size is an appropriate name. When we say count I also think it could be the count of number of ledgers. Wdyt? cc @sydneynotthecity Please weigh in on naming convention for sample size.

@2opremio
Copy link
Contributor Author

@janewang how about feeCount?

@2opremio
Copy link
Contributor Author

Actually, we could have feeCount and ledgerCount

@sydneynotthecity
Copy link

Actually, we could have feeCount and ledgerCount

I still think feeCount is a little confusing for end users. I would prefer either sorobanTransactionCount or observationCount. So that it's more clear what fees are counted.

@psheth9 psheth9 added the api-change Any changes to existing API or addition of new API label May 15, 2024
2opremio added a commit to 2opremio/go-2 that referenced this pull request May 16, 2024
* Enable `ENABLE_DIAGNOSTICS_FOR_TX_SUBMISSION` when enforcing soroban diagnostics (needed for soroban-rpc)
* Add `EnforceSorobanTransactionMetaExtV1` toml parameter, so that `EMIT_SOROBAN_TRANSACTION_META_EXT_V1` is enabled by default (needed for soroban-rpcs new `getFeeStats` endpoint). See stellar/soroban-rpc#172
* Mock core version and protocol in tests so that toml generation is more realistic
2opremio added a commit to stellar/go that referenced this pull request May 16, 2024
* ingest/ledgerbackend: tweak enforcing toml parameters and tests

* Enable `ENABLE_DIAGNOSTICS_FOR_TX_SUBMISSION` when enforcing soroban diagnostics (needed for soroban-rpc)
* Add `EnforceSorobanTransactionMetaExtV1` toml parameter, so that `EMIT_SOROBAN_TRANSACTION_META_EXT_V1` is enabled by default (needed for soroban-rpcs new `getFeeStats` endpoint). See stellar/soroban-rpc#172
* Mock core version and protocol in tests so that toml generation is more realistic

* Appease go vet
@2opremio
Copy link
Contributor Author

Actually, we could have feeCount and ledgerCount

I still think feeCount is a little confusing for end users. I would prefer either sorobanTransactionCount or observationCount. So that it's more clear what fees are counted.

I will go with transactionCount

@2opremio 2opremio marked this pull request as ready for review May 16, 2024 22:00
@2opremio
Copy link
Contributor Author

2opremio commented May 16, 2024

This is ready for review. @tamirms @sydneynotthecity @janewang PTAL

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-change Any changes to existing API or addition of new API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide getFeeStats as a new endpoint on RPC
6 participants