fix: change validation of tokenfactory to change admin to ""#324
Merged
Conversation
Contributor
WalkthroughThe pull request fixes a validation issue in the tokenfactory module's change admin functionality. The ValidateBasic method in MsgChangeAdmin was updated to treat the NewAdmin parameter as optional, only validating it as a Bech32 address when a non-empty string is provided. A regression test was added to verify that transactions with empty NewAdmin strings are now accepted through the full transaction pipeline. The changelog was updated to document this fix. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
x/tokenfactory/keeper/admins_test.go (1)
437-443:⚠️ Potential issue | 🟠 MajorTest assertion bug: expecting empty string but admin is set to unreachable address.
The test case at lines 380-390 sets the admin to
"kii1qqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqm6xlkr", but the assertion logic at line 440 expects the admin to be an empty string""whenexpectedAdminIndex == -1. This test will fail.The comment on line 437 is also stale and refers to the old behavior.
🐛 Proposed fix
- // expectedAdminIndex with negative value is assumed as admin with value of "" - const emptyStringAdminIndexFlag = -1 - if tc.expectedAdminIndex == emptyStringAdminIndexFlag { - suite.Require().Equal("", queryRes.AuthorityMetadata.Admin) + // expectedAdminIndex with negative value is assumed as admin with unreachable address + const unreachableAdminIndexFlag = -1 + if tc.expectedAdminIndex == unreachableAdminIndexFlag { + suite.Require().Equal("kii1qqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqm6xlkr", queryRes.AuthorityMetadata.Admin) } else { suite.Require().Equal(suite.TestAccs[tc.expectedAdminIndex].String(), queryRes.AuthorityMetadata.Admin) }Alternatively, consider adding an
expectedAdminfield to the test case struct for more explicit control over the expected admin value.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x/tokenfactory/keeper/admins_test.go` around lines 437 - 443, The assertion assumes expectedAdminIndex == -1 means the admin should be the empty string, but the test case actually sets admin to the unreachable address "kii1...m6xlkr", so update the test to assert the real expected admin rather than "" — either extend the test case struct with an explicit expectedAdmin field and use queryRes.AuthorityMetadata.Admin == tc.expectedAdmin when present, or change the negative-index branch to compare against the actual unreachable address value (referencing expectedAdminIndex, emptyStringAdminIndexFlag, and queryRes.AuthorityMetadata.Admin); also remove or update the stale comment about old behavior.
🧹 Nitpick comments (2)
x/tokenfactory/keeper/admins_test.go (1)
102-103: Consider extracting the unreachable address to a constant.The address
"kii1qqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqm6xlkr"is repeated in multiple places (lines 103, 109, 113, 382). Extracting it to a named constant would improve readability and maintainability.♻️ Optional refactor
Add a constant at the package or test level:
const unreachableAdmin = "kii1qqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqm6xlkr"Then use
unreachableAdminin place of the hardcoded string throughout the tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x/tokenfactory/keeper/admins_test.go` around lines 102 - 103, Extract the repeated unreachable address string into a named constant (e.g., unreachableAdmin) and replace all inline occurrences used in tests when constructing the ChangeAdmin message; locate references in the test where suite.msgServer.ChangeAdmin is called with types.NewMsgChangeAdmin(suite.TestAccs[1].String(), suite.defaultDenom, "<addr>") and substitute the literal address with the new unreachableAdmin constant so all occurrences (previously at the NewMsgChangeAdmin calls and other assertions) use the single constant for readability and maintainability.x/tokenfactory/README.md (1)
14-18: Documentation change looks good, but consider adding an example.The change correctly documents the behavior of using an unreachable address instead of an empty string. However, users might benefit from a brief example or clarification of what constitutes an "unreachable address" (e.g., a valid bech32 address with no corresponding private key, like the one used in tests:
kii1qqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqm6xlkr).📝 Optional: Add example to clarify
can choose to share admin privileges with other accounts using the authz module. The `ChangeAdmin` functionality, allows changing the master admin - account, or even setting it to an unreachable address, meaning no account has admin privileges + account, or even setting it to an unreachable address (e.g., `kii1qqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqm6xlkr`), meaning no account has admin privileges of the asset.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x/tokenfactory/README.md` around lines 14 - 18, Add a short example and clarification to the ChangeAdmin documentation: explain what is meant by an "unreachable address" (a syntactically valid bech32 address with no known private key) and include a concrete example address such as kii1qqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqm6xlkr; update the README paragraph referencing ChangeAdmin/admin to append one or two sentences showing the sample unreachable bech32 and noting it will effectively revoke admin privileges because no key can sign transactions for that address.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@x/tokenfactory/keeper/admins_test.go`:
- Around line 111-116: Update the stale test comment that mentions "empty admin"
to reflect the current test input of an unreachable address: adjust the comment
above the DenomsFromAdmin call (where suite.queryClient.DenomsFromAdmin is
invoked with QueryDenomsFromAdminRequest.Admin set to "kii1qq...m6xlkr") to say
"unreachable address" (or equivalent) so the comment matches the test intent
checked by adminRes and suite.Require assertions.
---
Outside diff comments:
In `@x/tokenfactory/keeper/admins_test.go`:
- Around line 437-443: The assertion assumes expectedAdminIndex == -1 means the
admin should be the empty string, but the test case actually sets admin to the
unreachable address "kii1...m6xlkr", so update the test to assert the real
expected admin rather than "" — either extend the test case struct with an
explicit expectedAdmin field and use queryRes.AuthorityMetadata.Admin ==
tc.expectedAdmin when present, or change the negative-index branch to compare
against the actual unreachable address value (referencing expectedAdminIndex,
emptyStringAdminIndexFlag, and queryRes.AuthorityMetadata.Admin); also remove or
update the stale comment about old behavior.
---
Nitpick comments:
In `@x/tokenfactory/keeper/admins_test.go`:
- Around line 102-103: Extract the repeated unreachable address string into a
named constant (e.g., unreachableAdmin) and replace all inline occurrences used
in tests when constructing the ChangeAdmin message; locate references in the
test where suite.msgServer.ChangeAdmin is called with
types.NewMsgChangeAdmin(suite.TestAccs[1].String(), suite.defaultDenom,
"<addr>") and substitute the literal address with the new unreachableAdmin
constant so all occurrences (previously at the NewMsgChangeAdmin calls and other
assertions) use the single constant for readability and maintainability.
In `@x/tokenfactory/README.md`:
- Around line 14-18: Add a short example and clarification to the ChangeAdmin
documentation: explain what is meant by an "unreachable address" (a
syntactically valid bech32 address with no known private key) and include a
concrete example address such as kii1qqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqm6xlkr;
update the README paragraph referencing ChangeAdmin/admin to append one or two
sentences showing the sample unreachable bech32 and noting it will effectively
revoke admin privileges because no key can sign transactions for that address.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f2164996-7210-47d4-a276-b44221e4c04c
📒 Files selected for processing (2)
x/tokenfactory/README.mdx/tokenfactory/keeper/admins_test.go
jhelison
reviewed
Apr 8, 2026
Thaleszh
force-pushed
the
docs/tokenfactory-change-admin
branch
from
d1ede89 to
c936557
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
x/tokenfactory/keeper/msg_server_test.go (3)
290-292: Rename the test to match actual expected behavior.The name says “BlockedByFullPipeline,” but the assertion expects success (
Code == 0). A name aligned with acceptance will reduce confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x/tokenfactory/keeper/msg_server_test.go` around lines 290 - 292, The test function name TestTF01_ChangeAdminEmptyStringBlockedByFullPipeline is misleading because the assertions expect success; rename the test function to reflect accepted behavior (for example TestTF01_ChangeAdminEmptyStringAcceptedByFullPipeline or TestTF01_ChangeAdminEmptyStringAllowed) and update any references to that symbol so the test name matches the assertion that the operation succeeds.
345-347: Assert the post-state admin value, not only tx acceptance.Lines 346-347 verify the tx is accepted, but the regression target is admin representation. Add an assertion on stored denom authority/admin to confirm it is the expected unreachable value (and not empty).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x/tokenfactory/keeper/msg_server_test.go` around lines 345 - 347, The test currently only checks tx acceptance via txResult/ blockRes but must also assert the stored denom's authority/admin is the expected unreachable value; after the existing tx acceptance assertion, fetch the stored denom authority/admin from the keeper for the denom used in this test (query the module's keeper for the denom metadata/authority) and add a suite.Require().Equal(expectedAdminValue, storedAdmin, "stored admin should be the unreachable value, not empty") to ensure the admin field matches the expected unreachable representation rather than being empty.
321-323: Use a fixed seed instead of wall-clock time for deterministic test behavior.Line 322 seeds randomness with
time.Now().UnixNano(), making test failures harder to reproduce. Use a fixed seed like1for consistent test runs.Suggested fix
signedTx, err := simtestutil.GenSignedMockTx( - rand.New(rand.NewSource(time.Now().UnixNano())), + rand.New(rand.NewSource(1)),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x/tokenfactory/keeper/msg_server_test.go` around lines 321 - 323, The test currently seeds randomness with time.Now().UnixNano() when calling simtestutil.GenSignedMockTx which makes test runs non-deterministic; change the seed to a fixed value (e.g., 1) by replacing rand.New(rand.NewSource(time.Now().UnixNano())) with rand.New(rand.NewSource(1)) where signedTx is created so GenSignedMockTx uses a deterministic RNG for reproducible tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@x/tokenfactory/types/msgs.go`:
- Around line 219-223: The ValidateBasic() logic currently skips address
validation when m.NewAdmin == "" which conflicts with tests expecting empty
NewAdmin to be invalid; update ValidateBasic() in msgs.go so it explicitly
rejects an empty m.NewAdmin (e.g., return an ErrInvalidAddress or similar when
m.NewAdmin == "") instead of only validating non-empty strings, and keep
setAdmin() as-is (it can continue to persist the provided value) so storage
doesn't receive an unintended empty admin; alternatively, if the intended
behavior is allowing empty admins, update the test expectation instead—pick one
and make the code/tests consistent by changing ValidateBasic() or the test
accordingly.
---
Nitpick comments:
In `@x/tokenfactory/keeper/msg_server_test.go`:
- Around line 290-292: The test function name
TestTF01_ChangeAdminEmptyStringBlockedByFullPipeline is misleading because the
assertions expect success; rename the test function to reflect accepted behavior
(for example TestTF01_ChangeAdminEmptyStringAcceptedByFullPipeline or
TestTF01_ChangeAdminEmptyStringAllowed) and update any references to that symbol
so the test name matches the assertion that the operation succeeds.
- Around line 345-347: The test currently only checks tx acceptance via
txResult/ blockRes but must also assert the stored denom's authority/admin is
the expected unreachable value; after the existing tx acceptance assertion,
fetch the stored denom authority/admin from the keeper for the denom used in
this test (query the module's keeper for the denom metadata/authority) and add a
suite.Require().Equal(expectedAdminValue, storedAdmin, "stored admin should be
the unreachable value, not empty") to ensure the admin field matches the
expected unreachable representation rather than being empty.
- Around line 321-323: The test currently seeds randomness with
time.Now().UnixNano() when calling simtestutil.GenSignedMockTx which makes test
runs non-deterministic; change the seed to a fixed value (e.g., 1) by replacing
rand.New(rand.NewSource(time.Now().UnixNano())) with rand.New(rand.NewSource(1))
where signedTx is created so GenSignedMockTx uses a deterministic RNG for
reproducible tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d1a35f5b-dc21-48d8-b663-de709b2fefed
📒 Files selected for processing (3)
CHANGELOG.mdx/tokenfactory/keeper/msg_server_test.gox/tokenfactory/types/msgs.go
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
Thaleszh
changed the title
jhelison
approved these changes
Apr 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
docs: document tokenfactory change admin to unreachable address instead of ""
Type of change