feat: add initial txfees logic #3956

gfanton · 2025-03-17T09:54:22Z

address: #3782

This PR implements the foundation for transaction fee logic. Notably, it:

Creates a base system GNO package for fee distribution and collection.
Adds a FeeCollector parameter to the auth module and a proposal function to update it.
Updates the Deposit function to correctly transfer fees to the active fee collector address.

I'm unsure about the placement/name, but the proposal I made made sense to me based on the actual context of the Account Keeper and the placeholder for the fee collector.

NOTE: It was my first time using Params. It wasn't immediately clear where to set the initial value, and it seems that multiple sources of truth exist for some parameters. It seems a bit fastidious to add a new Param to the Params module. However, I assume modules are still missing or not implemented yet.

Gno2D2 · 2025-03-17T09:55:30Z

🛠 PR Checks Summary

All Automated Checks passed. ✅

Manual Checks (for Reviewers):

IGNORE the bot requirements for this PR (force green CI check)

✅ Automated Checks (for Contributors):

🟢 Maintainers must be able to edit this pull request (more info)

☑️ Contributor Actions:

Fix any issues flagged by automated checks.
Follow the Contributor Checklist to ensure your PR is ready for review.
- Add new tests, or document why they are unnecessary.
- Provide clear examples/screenshots, if necessary.
- Update documentation, if required.
- Ensure no breaking changes, or include BREAKING CHANGE notes.
- Link related issues/PRs, where applicable.

☑️ Reviewer Actions:

Complete manual checks for the PR, including the guidelines and additional checks if applicable.

📚 Resources:

Debug

Automated Checks
Maintainers must be able to edit this pull request (more info)
If
🟢 Condition met
└── 🟢 And
    ├── 🟢 The base branch matches this pattern: ^master$
    └── 🟢 The pull request was created from a fork (head branch repo: gfanton/gno)
Then
🟢 Requirement satisfied
└── 🟢 Maintainer can modify this pull request
Manual Checks
**IGNORE** the bot requirements for this PR (force green CI check)
If
🟢 Condition met
└── 🟢 On every pull request
Can be checked by

Any user with comment edit permission

codecov · 2025-03-17T10:04:28Z

Codecov Report

Attention: Patch coverage is 78.09524% with 23 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
gno.land/pkg/gnoland/genesis.go	77.19%	10 Missing and 3 partials ⚠️
gno.land/pkg/integration/testscript_gnoland.go	79.31%	4 Missing and 2 partials ⚠️
tm2/pkg/sdk/auth/keeper.go	42.85%	3 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

gfanton · 2025-03-17T12:59:27Z

gno.land/pkg/integration/testscript_gnoland.go

 			genesis.VM.RealmParams = append(genesis.VM.RealmParams, tsGenesis.VM.RealmParams...)
+			genesis.Auth.Params.FeeCollector = tsGenesis.Auth.Params.FeeCollector


I should be able to set all the parameter values here, but the genesis toml file should then contain all the default values for the Auth module.

the more default values you can set in the genesis_params.toml file (with comments), the better. it's doc.

That makes sense. I like having this genesis_params as the source of truth for gno.land params. Should we apply them with a right merge? It would be better to have the file override the default values set in params.go files instead of having the file replace all the values, even empty one.

zivkovicmilos · 2025-03-18T10:59:21Z

examples/gno.land/r/sys/txfees/render.gno

+	output += "# Fee Coinbase\n"
+	output += "Balance: " + balance + "\n\n"
+
+	// XXX: Get the actual param value


What do you mean?

we can't; is the info really useful? if yes, we can import a contract a get the value.

What do you mean?

I simply want to show current fee collector address.
There is no way right now to get params values from the gno files perspective.

we can't; is the info really useful?

I think it can be useful; if updated, there is no easy and straightforward way from a user perspective to get this info (except with gnokey, which is not something I would call user-friendly).

if yes, we can import a contract a get the value.

wdym? If you mean setting the value while updating the param, I'm not a big fan of this idea because we cannot get the initial value of the param, and in specific, this context, the initial value can stay there for a while.

zivkovicmilos · 2025-03-18T11:02:29Z

gno.land/genesis/genesis_params.toml

@@ -12,6 +12,9 @@
  # TODO: chain_tz = "UTC"
  # TODO: default_storage_allowance = ""

+["auth"]
+  fee_collector = "g1najfm5t7dr4f2m38cg55xt6gh2lxsk92tgh0xy" # "gno.land/r/sys/txfees"


You should set this in the auth/params.go, this file (genesis_params.toml) is just for filetests (testing)

And I think the choice for the coinbase in auth is alright for now 👍
cc @moul

You should set this in the auth/params.go, this file (genesis_params.toml) is just for filetests (testing)

hmmm, that's a bit confusing. Maybe we should rename it then? But it made sense to have this file living in gno.land, setting all default parameter values for gno.land. For example, setting FeeCollector to sys/txfees is something only gno.land should be aware of, so it makes sense to have this file describing the default values of gno.land Params. (see my other comments)

auth/params.go is where you define the module's suggested default values for the chain it will run on.

genesis_params.toml is not intended for file tests; rather, it customizes the blockchain genesis for the gno.land chain. It may seem counterintuitive to have two default values. However:

The values in */params.go and genesis_params.toml can differ. For example, we might expect most tm2 chains to have a block time of 8 seconds, but we may want gno.land-based chains to have a shorter default block time.

The genesis_params.toml file in the monorepo acts as documentation, similar to the Tendermint default configuration file, which is filled with comments. This file usually contains default values that can be skipped but makes it easier for users to update a value without searching for a key.

Please specify in both locations.

tm2/pkg/sdk/auth/keeper.go

tm2/pkg/sdk/auth/params.go

tm2/pkg/sdk/bank/params.go

examples/gno.land/r/sys/txfees/render.gno

tm2/pkg/sdk/auth/consts.go

moul · 2025-03-18T15:13:08Z

tm2/pkg/sdk/auth/keeper.go

+func (ak AccountKeeper) FeeCollectorAddress(ctx sdk.Context) crypto.Address {
+	feeCollector := ak.GetParams(ctx).FeeCollector
+	if feeCollector.IsZero() {
+		feeCollector = crypto.AddressFromPreimage([]byte(DefaultFeeCollectorName))


Just panic if there is no fee collector. Your current implementation isn't caching the case of IsZero. Let's explicitly panic so that blockchain builders can define something custom or just keep the default value, which should work.

I'm fine with panicing, but I'm concerned about the fact that NewAccountKeeper doesn't initialize DefaultParam, which means that by default, calling FeeCollectorAddress on a fresh account keeper from NewAccountKeeper will panic.

gno.land/pkg/gnoland/genesis.go

moul

It seems to be heading in a good direction. Please continue and let me know when you're finished.

(blocking so i can review)

feat: add initial txfees logic

da7c04d

gfanton requested a review from zivkovicmilos March 17, 2025 09:54

github-actions bot assigned gfanton Mar 17, 2025

gfanton requested a review from moul March 17, 2025 09:54

github-actions bot added 🧾 package/realm Tag used for new Realms or Packages. 📦 🌐 tendermint v2 Issues or PRs tm2 related 📦 ⛰️ gno.land Issues or PRs gno.land package related labels Mar 17, 2025

fix: add missing params

9041ed1

gfanton commented Mar 17, 2025

View reviewed changes

gfanton added 3 commits March 17, 2025 14:29

chore: prefix sys txtar

df5643c

chore: lint + test

6f2cc00

Merge branch 'master' into feat/txfee

524d059

Kouteki added this to the 🚀 Mainnet beta launch milestone Mar 17, 2025

zivkovicmilos reviewed Mar 18, 2025

View reviewed changes