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

Ensure all estimates below feeMinimum are excluded #54

Merged
merged 1 commit into from
Apr 23, 2024

Conversation

mrfelton
Copy link
Member

@mrfelton mrfelton commented Apr 23, 2024

This PR addresses an issue in which invalid estimates (estimates below the configured fee minimum) were treated as valid if they came from the determined most relevant data providers (provider with the highest block height). In reality, even the provider with the highest block height can include invalid estimates that need to be filtered out.

This pull request introduces changes to the DataProviderManager class in the src/lib/DataProviderManager.ts file and its associated tests. The changes mainly involve the refactoring of how fee estimates are handled, with the introduction of a new method to filter fee estimates, and changes to the merging of fee estimates. The tests have also been updated to reflect these changes.

Refactoring of fee estimates handling:

  • src/lib/DataProviderManager.ts: The comment in the fee calculation section was updated to reflect the new fee formatting approach. The conditional logic for applying the fee multiplier was removed, and the warning log for estimates below the minimum fee was also removed. A new private method filterEstimates was introduced to handle the filtering of fee estimates based on the fee minimum. This method also logs a warning for estimates below the minimum fee. [1] [2]

Changes to merging of fee estimates:

  • src/lib/DataProviderManager.ts: The mergeFeeEstimates method was modified to initialize mergedEstimates as an empty object and to use the new filterEstimates method when iterating over data points. The condition for adding an estimate to mergedEstimates was also updated to account for cases where an estimate for a block target does not yet exist in mergedEstimates. [1] [2]

Updates to tests:

  • test/DataProviderManager-merge.test.ts: A new test file was added to specifically test the merging of fee estimates from multiple providers. Mock providers were created and registered to a DataProviderManager instance, and a test was added to check that the fee estimates are merged correctly.

  • test/DataProviderManager-sort.test.ts: This file was renamed from test/DataProviderManager.test.ts. The mock providers in this file were updated to return empty fee estimates, and the test for merging fee estimates was removed.

@mrfelton mrfelton added the type:bug Indicates an unexpected problem or unintended behavior label Apr 23, 2024
@mrfelton mrfelton self-assigned this Apr 23, 2024
@mrfelton mrfelton merged commit 17399e4 into master Apr 23, 2024
1 check passed
@mrfelton mrfelton deleted the fix/fee-min-filter branch April 23, 2024 09:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants