Skip to content
This repository has been archived by the owner on Jan 22, 2025. It is now read-only.

Deprecate FeeCalculator returning APIs #19120

Merged

Conversation

jackcmay
Copy link
Contributor

@jackcmay jackcmay commented Aug 9, 2021

Problem

FeeCalculator is directly exposed via external APIs limiting the evolution of how Solana calculates fees.

Summary of Changes

  • Deprecate APIs that expose FeeCalculator
  • Expose new APIs that return just the blockhash, or allow a fee to be calculated based on a passed in Message
  • Rework internal mechanisms to use the new APIs

These changes do not change how Solana calculates fees, but instead encapsulates the fee calculations to the bank so that they may evolve over time.

Outstanding work:

  • Rework downstream projects to use the new APIs
  • More tests

Fixes #

@jackcmay jackcmay added the work in progress This isn't quite right yet label Aug 9, 2021
@jackcmay jackcmay force-pushed the deprecate-feecalcalulator-returning-apis branch 5 times, most recently from 274ec8e to 04032c8 Compare August 10, 2021 00:02
@jackcmay jackcmay removed the work in progress This isn't quite right yet label Aug 10, 2021
@jackcmay jackcmay force-pushed the deprecate-feecalcalulator-returning-apis branch 2 times, most recently from a8f171f to 8b66dd4 Compare August 10, 2021 05:54
@jackcmay jackcmay force-pushed the deprecate-feecalcalulator-returning-apis branch 3 times, most recently from f38c361 to 5818a70 Compare August 11, 2021 19:19
@jackcmay jackcmay requested a review from t-nelson August 11, 2021 19:19
@jackcmay
Copy link
Contributor Author

@t-nelson Would you take a look at least at the nonce/offline signing related stuff?

@codecov
Copy link

codecov bot commented Aug 11, 2021

Codecov Report

Merging #19120 (19534e4) into master (5233338) will decrease coverage by 0.0%.
The diff coverage is 72.5%.

@@            Coverage Diff            @@
##           master   #19120     +/-   ##
=========================================
- Coverage    82.9%    82.8%   -0.1%     
=========================================
  Files         455      455             
  Lines      129154   129413    +259     
=========================================
+ Hits       107159   107278    +119     
- Misses      21995    22135    +140     

Copy link
Contributor

@CriesofCarrots CriesofCarrots left a comment

Choose a reason for hiding this comment

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

Looking good! Just one consideration about the various get_fee_for_transaction methods that take a Message...
Otherwise, nitty nits. (Btw, the nonce stuff I only skimmed, since Trent will be looking at that closely)

Copy link
Contributor

@t-nelson t-nelson left a comment

Choose a reason for hiding this comment

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

Nonce/offline stuff looks great. Just one comment from me beyond Tyera's notes

@jackcmay jackcmay force-pushed the deprecate-feecalcalulator-returning-apis branch from 1fd73e7 to 8d5a44c Compare August 12, 2021 17:11
@jackcmay jackcmay merged commit 0b50bb2 into solana-labs:master Aug 13, 2021
@jackcmay jackcmay deleted the deprecate-feecalcalulator-returning-apis branch August 13, 2021 16:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants