fix: crosscheck UpdateTokenMetadata decimals with ERC20 contracts and bank metadata#325
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds pre-update validation in UpdateFeeTokens to ensure each UpdateTokenMetadata.Decimals matches canonical metadata: for ERC20-prefixed denoms it queries the ERC20 contract via QueryERC20 and verifies contract decimals; for non-ERC20 denoms it checks bank denom metadata via GetDenomMetaData and compares the max denom unit exponent. Validation rejects mismatches with an error; bank denoms without metadata are skipped. A new unexported validateFeeTokenDecimals helper centralizes checks. Keeper interfaces gained QueryERC20 and GetDenomMetaData methods. Tests exercising ERC20, bank, and unknown-denom paths were added. CHANGELOG updated. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Summary
🚥 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/feeabstraction/keeper/msg_server.go`:
- Around line 173-176: The code path in msg_server.go currently skips validation
when ms.bankKeeper.GetDenomMetaData(ctx, token.Denom) returns !found; change
this to fail-closed by returning an error (reject the update) unless metadata
exists, or implement an explicit allowlist lookup before bypassing;
specifically, in the handler that calls ms.bankKeeper.GetDenomMetaData for
token.Denom, replace the unconditional "return nil" on !found with a rejection
(e.g., return an sdk error) or consult an explicit allowlist (e.g., a
function/isAllowedNativeDenom) and only bypass if the denom is in that
allowlist. Ensure the error message clearly references the missing metadata and
the denom.
🪄 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: 11dec457-a6a3-4f4d-9a7b-1a19c3836c3a
📒 Files selected for processing (4)
CHANGELOG.mdx/feeabstraction/keeper/msg_server.gox/feeabstraction/keeper/msg_server_test.gox/feeabstraction/types/expected_keepers.go
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Description
Crosscheck UpdateTokenMetadata decimals with ERC20 contracts and bank metadata to avoid any possible mismatch
Type of change
How Has This Been Tested?
Added simple checks for both ERC20 and bank metadata infos
PR Checklist:
Make sure each step was done:
make lint-fix