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

chore: Use floats for fee multipliers, add base multiplier #14818

Merged
merged 5 commits into from
Mar 13, 2025

Conversation

janjakubnanista
Copy link
Contributor

@janjakubnanista janjakubnanista commented Mar 12, 2025

Description

The fee estimator for devnet-sdk had a bit of a usability issue with it - the base & tip multipliers were defined as big.Ints which meant the fees could only be increased by integer multiples. This is not great as there are plenty of numbers between 1 and 2 (some would even say an infinite amount of numbers but those people are not working with float64 limitations).

This PR fixes the issue and adds base multiplier besides the tip multiplier. Some libraries set this to 1.2 by default (ethers, viem) but to avoid any surprises, a value of 1.0 will be used for both of these by default.

On top of the changes in the logic, the functional options pattern has been used (thanks @scharissis for this)

@janjakubnanista janjakubnanista added the A-devnet-sdk Area: devnet-sdk label Mar 12, 2025
@janjakubnanista janjakubnanista self-assigned this Mar 12, 2025
@janjakubnanista janjakubnanista requested a review from a team as a code owner March 12, 2025 17:04
Copy link

codecov bot commented Mar 12, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 41.92%. Comparing base (f81a2e3) to head (77107be).
Report is 6 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #14818      +/-   ##
===========================================
- Coverage    46.48%   41.92%   -4.56%     
===========================================
  Files         1046      874     -172     
  Lines        90907    80759   -10148     
===========================================
- Hits         42254    33862    -8392     
+ Misses       45547    43966    -1581     
+ Partials      3106     2931     -175     
Flag Coverage Δ
cannon-go-tests-32 ?
cannon-go-tests-64 ?
contracts-bedrock-tests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

see 178 files with indirect coverage changes

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Contributor

@scharissis scharissis left a comment

Choose a reason for hiding this comment

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

Just flagging what I think is non-idiomatic Go; not a hard blocker for me if you need to merge soon. (But worth chatting about here or elsewhere!)

@janjakubnanista
Copy link
Contributor Author

@scharissis made some changes, could you please re-check?

@janjakubnanista janjakubnanista added this pull request to the merge queue Mar 13, 2025
Merged via the queue into develop with commit 8f98334 Mar 13, 2025
49 checks passed
@janjakubnanista janjakubnanista deleted the jan/base-fee-multiplier branch March 13, 2025 22:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-devnet-sdk Area: devnet-sdk
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants