fix: respect storage deposit denom

[julienrbrt]

Custom storage deposit price is supported via config, but the VM keeper was hardcoding its denon to ugnot.Denom. If the storage price uses another denom it won't be respected.
Additionally, improve the error message when getting to low amount for storage.

[Gno2D2]

🛠 PR Checks Summary

All Automated Checks passed. ✅

Manual Checks (for Reviewers):

• IGNORE the bot requirements for this PR (force green CI check)
• The pull request description provides enough details

Read More

🤖 This bot helps streamline PR reviews by verifying automated checks and providing guidance for contributors and reviewers.

✅ Automated Checks (for Contributors):

🟢 Maintainers must be able to edit this pull request (more info)
🟢 Pending initial approval by a review team member, or review from tech-staff

☑️ Contributor Actions:

1. Fix any issues flagged by automated checks.
2. 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:

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

📚 Resources:

• Report a bug with the bot.
• View the bot’s configuration file.

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: julienrbrt/gno)

Then

🟢 Requirement satisfied
└── 🟢 Maintainer can modify this pull request

Pending initial approval by a review team member, or review from tech-staff

If

🟢 Condition met
└── 🟢 And
    ├── 🟢 The base branch matches this pattern: ^master$
    └── 🟢 Not (🔴 Pull request author is a member of the team: tech-staff)

Then

🟢 Requirement satisfied
└── 🟢 If
    ├── 🟢 Condition
    │   └── 🟢 Or
    │       ├── 🔴 At least one of these user(s) reviewed the pull request: [jefft0 leohhhn n0izn0iz notJoon omarsy x1unix] (with state "APPROVED")
    │       ├── 🟢 At least 1 user(s) of the team tech-staff reviewed pull request
    │       └── 🔴 This pull request is a draft
    └── 🟢 Then
        └── 🟢 Not (🔴 This label is applied to pull request: review/triage-pending)

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

The pull request description provides enough details

If

🟢 Condition met
└── 🟢 And
    ├── 🟢 Not (🔴 Pull request author is a member of the team: core-contributors)
    └── 🟢 Not (🔴 Pull request author is user: dependabot[bot])

Can be checked by

• team core-contributors

[codecov]

Codecov Report

❌ Patch coverage is 72.72727% with 6 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
gno.land/pkg/sdk/vm/keeper.go	62.50%	6 Missing ⚠️

📢 Thoughts on this report? Let us know!

[piux2]

Custom denom is supported via config, but the VM keeper was hardcoding ugnot.Denom. Additionally, improve the error message when getting to low amount for storage.

@julienrbrt thank you for the PR.

Could you please provide some context on why we want to have a custom denom in gno.land and how it will be used for storage deposits?

Everything implemented in gno.land/pkg/sdk/vm/keeper.go is specific to the gno.land chain. Since storage is provided by the chain itself, we’ve chosen to use the native GNO token as deposit payment for it.

Since parameters can be modified by the GovDAO, there are more complex scenarios we need to consider when the deposit token is changed — especially for users who have already locked their deposits for storage usage.

[julienrbrt]

Everything implemented in gno.land/pkg/sdk/vm/keeper.go is specific to the gno.land chain. Since storage is provided by the chain itself, we’ve chosen to use the native GNO token as deposit payment for it.

Since parameters can be modified by the GovDAO, there are more complex scenarios we need to consider when the deposit token is changed — especially for users who have already locked their deposits for storage usage.

Correct but in this scenario the parameter could be defined as another denom, but that wouldn't be reflected here with the current code.

I would classify it as a bug, as indeed if GovDAO were to change the denom it wouldn't be reflected properly, while it should.

For gno.land it probably wouldn't matter, as the denom will most likely always be ugnot, but it is still weird to ignore the denom.

[piux2]

Correct but in this scenario the parameter could be defined as another denom, but that wouldn't be reflected here with the current code.
I would classify it as a bug, as indeed if GovDAO were to change the denom it wouldn't be reflected properly, while it should.
For gno.land it probably wouldn't matter, as the denom will most likely always be ugnot, but it is still weird to ignore the denom.

In the main branch, we use ugnot.Denom for the gno.land storage deposit. The parameter does not currently include a storage denom, so the GovDAO cannot modify it.

This PR breaks the above assumption on gno.land. More importantly, we need to first determine how token exchange rates would affect storage pricing and potential storage arbitrage before enabling custom denoms.

Could you provide some context on why we want to use a different token instead of ugnot for the deposit, and how that token would be used as storage deposit on gno.land?

[moul]

I see at least two potential scenarios where a gno.land chain might want to have another token:

• It could be an ICS zone that supports another existing chain's token, or even a dual token in the future.
• It might have different denoms depending on various deployments, such as "$TESTGNOT" for a testnet, a "$FOOGNOT" for a foo.gno.land.

[piux2]

We should verify that DefaultDeposit is ugnot.Denom, since gno.land uses ugnot as the fixed storage deposit token rather than a modifiable one.

[julienrbrt]

This is ensured at param validation already. It makes sure storage price and default deposit is the same denom (see params.go change here). Effectively forcing ugnot being used for gno.land. Any modification of those params denom will be blocked unless both are changed, which removes the arbitrage issue you've mentioned.

I still do not believe the keeper needs it hardcoded when the protections are in place.

[piux2]

It makes sure storage price and default deposit is the same denom (see params.go change here).... Any modification of those params denom will be blocked unless both are changed, which removes the arbitrage issue you've mentioned.

Could you point out where in the code this logic—blocking changes unless both fields are modified—is enforced?

More importantly, how does ensuring that both DefaultDeposit and StoragePrice remain consistent actually prevent users from using a denom other than ugnot for storage deposits on Gno.land?

Modifying params.DefaultDeposit happens through a separate GovDAO voting process. Also, params.DefaultDeposit isn't a global singleton—it only exists within the context of a transaction

Once a vote approves setting vm/params.DefaultDeposit to 1000bar, all transactions will use 1000 bar as the default deposit.

If we want to enforce our assumption that gno.land uses only ugnot as the storage deposit denom, we need to enforce that in two places:

1. Validation in DefaultDeposit and StoragePrice params: This prevents GovDAO from passing invalid modifications.

gno/gno.land/pkg/sdk/vm/params.go

Line 70 in c93683d

coins, err := std.ParseCoins(p.DefaultDeposit)

gno/gno.land/pkg/sdk/vm/params.go

Line 74 in c93683d

coins, err = std.ParseCoins(p.StoragePrice)

gno/gno.land/pkg/sdk/vm/params.go

Line 121 in c93683d

// XXX validate input?

2. Enforcement in processStorageDeposit(): To ensure that only ugnot is used as the denom for deposit, defaulDeposit and storagePrice, we also enforce it at runtime. If a different denom is encountered for any reason, the function will panic to prevent unintended behavior.

[julienrbrt]

There is no ugnot denom hardcoded in this PR. Only the protection if a user would change the deposit denom without changing the storage denom. This solves the issue for a live gno.land chain, as the parameters changes would be refused. This still solves the issue at hand.

This PR will not hardcode the denom.

[piux2]

Only the protection if a user would change the deposit denom without changing the storage denom. This solves the issue for a live gno.land chain, as the parameters changes would be refused. This still solves the issue at hand.

Could you please point out which line in this PR prevents the GovDAO from updating parameters to use a non-ugnot denom?

Both DefaultDeposit and StoragePrice can be set by GovDAO to values like 600000000barrt and 100barrt through a separate transaction, and such updates would succeed. Then, if a user submits a message without explicitly setting the --max-deposit flag, the updated DefaultDeposit (e.g., max 600000000barrt) will apply. If the user holds barrt tokens, those will be used as the deposit.

[julienrbrt]

params.Validate() won't let it happen in two transactions.

[piux2]

The storage deposit should use ugnot.Denom instead of deriving it from DefaultDeposit’s denom, which is modifiable as a parameter.

[piux2]

The storage deposit should use ugnot.Denom instead of deriving it from DefaultDeposit’s denom, which is modifiable as a parameter.

[julienrbrt]

If you consider the above blocking, feel free to close: #4852 (comment).
I will use a light fork containing this change only then... (https://github.com/allinbits/gno/tree/denom-fix)

cc @piux2 @moul @zivkovicmilos

[moul]

I'm in favor of moving forward, as I anticipate that all uses of these PR changes will be positive and easily manageable under safe conditions.

However, we can consider alternative approaches that align with Ray's request. One option could be to introduce a new dedicated parameter (avoid mixing). Another option would be to avoid using a parameter altogether and instead rely on a pure configuration parameter from the configuration file. I have a strong preference for parameters, as I expect GovDAO to be able to change the value to match their exact needs.

[julienrbrt]

Another option would be to avoid using a parameter altogether and instead rely on a pure configuration parameter from the configuration file.

I am not sure I understand what parameter is missing:

default_deposit is a token. so amount + denom.
storage_price is as well a token. so amount + denom.

Both are currently modifiable by GovDAO at genesis or via NewSysParamStringPropRequest.
Now this PR makes sure that default_deposit denom == storage_price denom.

[piux2]

Another option would be to avoid using a parameter altogether and instead rely on a pure configuration parameter from the configuration file.

I am not sure I understand what parameter is missing:

default_deposit is a token. so amount + denom. storage_price is as well a token. so amount + denom.

Both are currently modifiable by GovDAO at genesis or via NewSysParamStringPropRequest. Now this PR makes sure that default_deposit denom == storage_price denom.

Please check out the comments and examples. The default_deposit is used when a user does not supply the --max-deposit flag.
(#4852 (comment))

On the main branch, we operate under the assumption that only ugnot will be used as the denom on gno.land to prevent arbitrage opportunities. However, this is currently not enforced fully in code. The existing logic only checks the deposit denom against ugnot, which is insufficient to guarantee this behavior.

I was under the impression that we had agreed the PR should explicitly enforce the use of ugnot in code. Instead, this PR removes the ugnot check on deposits, which seems to move in the opposite direction — potentially signaling that changing the storage denom to something other than ugnot via GovDAO is acceptable.