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

X-chain - introducing fees calculators #2700

Open
wants to merge 139 commits into
base: master
Choose a base branch
from

Conversation

abi87
Copy link
Contributor

@abi87 abi87 commented Feb 2, 2024

Why this should be merged

This PR introduced fees.Calculator following closely what we have done in the platformvm.

How this works

  • Introduce fee.Calculator interface and staticCalculator
  • Moved fee related part of avm config to fee package

How this was tested

CI

@abi87 abi87 self-assigned this Feb 2, 2024
@abi87 abi87 linked an issue Feb 2, 2024 that may be closed by this pull request
@abi87 abi87 force-pushed the x-chain_introducing-fees-calculators branch 12 times, most recently from 4e0e2f8 to 53df271 Compare February 3, 2024 23:54
Copy link
Contributor

@tsachiherman tsachiherman left a comment

Choose a reason for hiding this comment

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

Does it make sense to have a function ?

func CalculateStaticFees(c StaticConfig, tx *txs.Tx) (uint64, error)

Or maybe you have future plans no included in this PR ?

@abi87
Copy link
Contributor Author

abi87 commented Jul 12, 2024

Does it make sense to have a function ?

func CalculateStaticFees(c StaticConfig, tx *txs.Tx) (uint64, error)

Or maybe you have future plans no included in this PR ?

Plan is to introduce dynamic fees as a struct implementing fee.Calculator, similarly to what will happen with the P-chain.
I reckon static fees do not need a whole struct to be calculated.
This is planning ahead a bit to handle uniformly fees pre and post fork introducing dynamic fees

}{
{
name: "BaseTx pre EUpgrade",
chainTime: preEUpgradeTime,
Copy link
Contributor

Choose a reason for hiding this comment

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

(No action required) Why is this parameter suggested for inclusion in the test case struct if it never varies?

}
}

func baseTx() txs.UnsignedTx {
Copy link
Contributor

Choose a reason for hiding this comment

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

(No action required) Maybe just inline these values given that they aren't used anywhere else?

@@ -0,0 +1,93 @@
// Copyright (C) 2019-2024, Ava Labs, Inc. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

(No action required) Should this be static_calculator_test.go?

vms/avm/txs/fee/static_calculator.go Show resolved Hide resolved
@abi87 abi87 mentioned this pull request Jul 17, 2024
5 tasks
Copy link

This PR has become stale because it has been open for 30 days with no activity. Adding the lifecycle/frozen label will cause this PR to ignore lifecycle events.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In Review 👀
Development

Successfully merging this pull request may close these issues.

X-chain - Dynamic Fees
3 participants