Skip to content

fix: enforce wasmbind uses msg server on tokenfactory set denom metadata path#329

Merged
Thaleszh merged 3 commits into
mainfrom
fix/wasmbind-capability
Apr 13, 2026
Merged

fix: enforce wasmbind uses msg server on tokenfactory set denom metadata path#329
Thaleszh merged 3 commits into
mainfrom
fix/wasmbind-capability

Conversation

@Thaleszh

Copy link
Copy Markdown
Contributor

Description

Enforce wasmbind uses msg server on tokenfactory set denom metadata path

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Added regression test

PR Checklist:

Make sure each step was done:

  • Updated changelog with PR's intent
  • Lint with make lint-fix

@Thaleszh Thaleszh requested a review from jhelison as a code owner April 13, 2026 17:40
@coderabbitai

coderabbitai Bot commented Apr 13, 2026

Copy link
Copy Markdown
Contributor

Walkthrough

This pull request refactors the PerformSetMetadata function in the wasmbinding module to delegate metadata operations to the token factory message server instead of directly manipulating state. The change removes the bankkeeper.Keeper parameter and ensures capability gating for EnableSetMetadata is enforced through the message server. Related callers are updated accordingly, and test coverage is added to validate the capability gating behavior.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~15 minutes

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and accurately summarizes the main change: enforcing that wasmbind uses the message server for tokenfactory's set denom metadata path, which aligns with the refactoring shown in the changeset.
Description check ✅ Passed The description is directly related to the changeset, explaining the purpose (enforce wasmbind uses msg server), the change type (bug fix), and noting that a regression test was added, all of which align with the actual changes made.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/wasmbind-capability

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
wasmbinding/tokenfactory/message_plugin.go (1)

66-66: Consider removing unused b bankkeeper.Keeper parameter from PerformCreateDenom.

After this refactor, the b parameter is no longer used inside PerformCreateDenom since PerformSetMetadata no longer requires it. The parameter is passed in from callers (line 58, and in tests) but serves no purpose now.

♻️ Proposed cleanup
-func PerformCreateDenom(f *tokenfactorykeeper.Keeper, b bankkeeper.Keeper, ctx sdk.Context, contractAddr sdk.AccAddress, createDenom *tfbindingtypes.CreateDenom) ([]byte, error) {
+func PerformCreateDenom(f *tokenfactorykeeper.Keeper, ctx sdk.Context, contractAddr sdk.AccAddress, createDenom *tfbindingtypes.CreateDenom) ([]byte, error) {

This would also require updating callers in CreateDenom (line 58) and test files.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@wasmbinding/tokenfactory/message_plugin.go` at line 66, Remove the now-unused
bankkeeper parameter from PerformCreateDenom: delete the `b bankkeeper.Keeper`
parameter from the PerformCreateDenom signature and update all callers (notably
the CreateDenom call site and any tests that pass a bankkeeper) to stop passing
that argument; also update any related function declarations/imports to reflect
the new signature (e.g., tests, helper calls) and run tests to ensure
compilation after removing the unused parameter.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@wasmbinding/tokenfactory/message_plugin.go`:
- Line 66: Remove the now-unused bankkeeper parameter from PerformCreateDenom:
delete the `b bankkeeper.Keeper` parameter from the PerformCreateDenom signature
and update all callers (notably the CreateDenom call site and any tests that
pass a bankkeeper) to stop passing that argument; also update any related
function declarations/imports to reflect the new signature (e.g., tests, helper
calls) and run tests to ensure compilation after removing the unused parameter.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 4bcabda2-cfdf-4c59-913f-71d82cdc49ea

📥 Commits

Reviewing files that changed from the base of the PR and between 09193f4 and c5a445b.

📒 Files selected for processing (3)
  • CHANGELOG.md
  • wasmbinding/tokenfactory/message_plugin.go
  • wasmbinding/tokenfactory/message_plugin_test.go

@codecov

codecov Bot commented Apr 13, 2026

Copy link
Copy Markdown

Codecov Report

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

Files with missing lines Patch % Lines
wasmbinding/tokenfactory/message_plugin.go 0.00% 6 Missing ⚠️

📢 Thoughts on this report? Let us know!

@Thaleszh Thaleszh merged commit b2b4ef5 into main Apr 13, 2026
9 of 10 checks passed
@Thaleszh Thaleszh deleted the fix/wasmbind-capability branch April 13, 2026 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants