-
Notifications
You must be signed in to change notification settings - Fork 662
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
P-chain: introducing dynamic fee calculator #3188
Conversation
a1b3a30
to
270ab49
Compare
…_block_level_fee_calculator
…abs/avalanchego into p-chain_block_level_fee_calculator
|
||
func ResetDynamicConfig(ctx *snow.Context, customFeesConfig *commonfee.DynamicFeesConfig) error { | ||
if customFeesConfig == nil { | ||
return nil // nothing to do |
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.
(No action required) Maybe make this an error condition?
buildingTx bool | ||
|
||
// outputs of visitor execution | ||
fee uint64 |
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.
(No action required) Maybe avoid using the visitor pattern and dispatch via other means (e.g. a switch statement)? This could enable a purely functional implementation of the fee calculation for each transaction type (i.e. no mutable state to maintain) which would be easier to test. Just inputs -> outputs for each fee calculation function and a test each for dispatch and meterTx.
return err | ||
} | ||
|
||
func (c *dynamicCalculator) meterTx( |
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.
(No action required) This method seems to represent the majority of the complexity of the calculator, but I don't see unit tests that target it directly. Maybe add explicit coverage? That should allow for both simpler per-type coverage and greater confidence that coverage is comprehensive with a minimum of complexity.
@@ -4,18 +4,84 @@ | |||
package fee |
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.
(No action required) I find it challenging to review this much test code in its current form. Might there be opportunities to break the testing into more reviewable chunks (e.g. by upgrade time/transaction type/???)? Coherent chunks of functionality or behavior with less boilerplate is generally easier to review and maintain, whether in the code under test or the tests themselves. The suggestion of using a more functional style might also help with this.
This has since been fully refactored and merged into master. |
Why this should be merged
Splitting introduction of dynamic fees into 4 PRs:
How this works
How this was tested
Extra UTs. All new code is not wired in yet. Will be done in a sequent PR