Simplify static fee calculations #3240
Conversation
|
|
||
| func (c *staticVisitor) RewardValidatorTx(*txs.RewardValidatorTx) error { | ||
| c.fee = 0 // no fees | ||
| c.fee = c.config.CreateSubnetTxFee | ||
| return nil | ||
| } |
There was a problem hiding this comment.
These are replaced with more explicit errors rather than returning no fees.
| AddSubnetDelegatorFee uint64 | ||
| NetworkID uint32 | ||
| AVAXAssetID ids.ID | ||
| StaticFeeConfig fee.StaticConfig |
There was a problem hiding this comment.
Now that the config doesn't have the CreateAssetFee - we can just use the config here directly 🥳
| chainTime time.Time | ||
| unsignedTx func() txs.UnsignedTx | ||
| expected uint64 | ||
| txTests = []struct { |
There was a problem hiding this comment.
These tests will be re-used for the dynamic fees tests.
| // NewStaticFeeCalculator creates a static fee calculator, with the config set | ||
| // to either the pre-AP3 or post-AP3 config. | ||
| func NewStaticFeeCalculator(cfg *config.Config, timestamp time.Time) fee.Calculator { | ||
| config := cfg.StaticFeeConfig | ||
| if !cfg.UpgradeConfig.IsApricotPhase3Activated(timestamp) { | ||
| config.CreateSubnetTxFee = cfg.CreateAssetTxFee | ||
| config.CreateBlockchainTxFee = cfg.CreateAssetTxFee | ||
| } | ||
| return fee.NewStaticCalculator(config) | ||
| } |
There was a problem hiding this comment.
I'm not convinced that this is the right place for these methods... But will leave that decision to a later PR.
StephenButtolph
requested review from
abi87,
marun,
danlaine and
dhrubabasu
as code owners
| unsignedTx: createChainTx, | ||
| expected: feeTestsDefaultCfg.CreateAssetTxFee, | ||
| name: "RewardValidatorTx", | ||
| tx: "0000000000143d0ad12b8ee8928edf248ca91ca55600fb383f07c32bff1d6dec472b25cf59a700000000", |
There was a problem hiding this comment.
(No action required) Maybe prefer using a non-opaque format for this kind of test data to simplify review and maintenance? Or is there a reason that tx decoding and parsing shouldn't be orthogonal to fee calculation?
There was a problem hiding this comment.
Do you think it would be better to move these transactions to separate files (and embed them?). These are really more intended to be compliance tests that should never change... I feel like if we included them in their non-binary forms... Their definition would dominate the tests... When really I think of them as amorphous tests to prevent unexpected changes.
There was a problem hiding this comment.
I added a TODO for this. I spent like 3 hours yesterday trying to come up with a nice way to implement this and I didn't make progress towards something I liked.
I don't want to block the rest of the PRs on this test definition.
Why this should be merged
The main goal of this PR is to simplify the
fee.NewStaticCalculatorsignature. Previously the signature was:Now it is:
How this works
This is done by removing
CreateAssetFeefrom theStaticConfigand expectingCreateSubnetTxFeeandCreateBlockchainTxFeeto be set by the caller.How this was tested