fix(oracle): use weight parameter in NewClaim constructor#288
Conversation
The NewClaim function accepted a weight parameter but incorrectly assigned the power value to the Weight field. This caused the Weight field to always equal Power, ignoring the intended weight argument. Fixed by setting Weight: weight instead of Weight: power.
WalkthroughA bug fix to the Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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
🧹 Nitpick comments (1)
x/tokenfactory/keeper/admins.go (1)
17-19: LGTM! Good fix to explicitly handle missing metadata.This correctly prevents
proto.Unmarshal(nil, ...)from silently returning an empty struct withAdmin="", which could mask authorization issues.Consider using a sentinel error for better caller discrimination between "not found" vs "unmarshal failure":
💡 Optional: Use sentinel error for better error handling
+var ErrNoAuthorityMetadata = errors.New("no authority metadata found") + func (k Keeper) GetAuthorityMetadata(ctx context.Context, denom string) (types.DenomAuthorityMetadata, error) { bz := k.GetDenomPrefixStore(sdk.UnwrapSDKContext(ctx), denom).Get([]byte(types.DenomAuthorityMetadataKey)) if bz == nil { - return types.DenomAuthorityMetadata{}, fmt.Errorf("no authority metadata found for denom: %s", denom) + return types.DenomAuthorityMetadata{}, fmt.Errorf("%w for denom: %s", ErrNoAuthorityMetadata, denom) }This allows callers (e.g., gRPC handlers, wasm bindings) to use
errors.Is(err, ErrNoAuthorityMetadata)to distinguish "not found" from other failures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x/tokenfactory/keeper/admins.go` around lines 17 - 19, Introduce a package-level sentinel error (e.g., var ErrNoAuthorityMetadata = errors.New("no authority metadata")) and replace the current nil-check return (the block that returns fmt.Errorf("no authority metadata found for denom: %s", denom) when bz == nil) with a wrapped sentinel error such as fmt.Errorf("%w: denom %s", ErrNoAuthorityMetadata, denom); update callers to discriminate using errors.Is(err, ErrNoAuthorityMetadata) where appropriate.
🤖 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/oracle/keeper/keeper.go`:
- Around line 269-278: RemoveExcessFeeds currently returns the last removal
error which fails tests and can halt the chain; change it to be non-failing:
iterate activesToClear, call k.ExchangeRate.Remove(ctx, denom), log any error
with ctx.Logger().Error (including denom and err) and do not assign or return an
error (return nil at end) so all errors are observable but block processing
continues; if you intend to propagate errors instead, replace the current logic
with an aggregator (collect all errors from k.ExchangeRate.Remove into a
combined error) and update the RemoveExcessFeeds tests to expect that aggregated
error.
---
Nitpick comments:
In `@x/tokenfactory/keeper/admins.go`:
- Around line 17-19: Introduce a package-level sentinel error (e.g., var
ErrNoAuthorityMetadata = errors.New("no authority metadata")) and replace the
current nil-check return (the block that returns fmt.Errorf("no authority
metadata found for denom: %s", denom) when bz == nil) with a wrapped sentinel
error such as fmt.Errorf("%w: denom %s", ErrNoAuthorityMetadata, denom); update
callers to discriminate using errors.Is(err, ErrNoAuthorityMetadata) where
appropriate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e66b27c0-1fb2-4b61-ae29-f81433ad420c
📒 Files selected for processing (3)
x/oracle/keeper/keeper.gox/oracle/types/ballot.gox/tokenfactory/keeper/admins.go
There was a problem hiding this comment.
Pull request overview
This PR adjusts tokenfactory authority-metadata retrieval behavior, fixes a bug in oracle claim construction, and changes oracle cleanup behavior when removing excess exchange-rate feeds.
Changes:
- Tokenfactory: return an error when authority metadata is missing for a denom.
- Oracle types: fix
NewClaimto setWeightfrom the providedweightparameter (instead ofpower). - Oracle keeper: change
RemoveExcessFeedsto return a (last) removal error instead of always returningnil.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| x/tokenfactory/keeper/admins.go | Adds missing-metadata handling to GetAuthorityMetadata. |
| x/oracle/types/ballot.go | Fixes incorrect field assignment in NewClaim. |
| x/oracle/keeper/keeper.go | Alters RemoveExcessFeeds error propagation semantics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
|
Hello @giwaov, we reviewed the issues you opened and some of them are not real, but some are! Can you take a look again on the issues that remained open? Also, feel free to discuss any of the issues we closed. After that, you will be needing to refactor both this PR and your other one, since they include changes we don't agree with (solving non-issues). |
6295109 to
019e9ee
Compare
|
Hey @Thaleszh, thanks for taking the time to review everything. Appreciate the thorough feedback. I've gone through the closed issues and the reasoning makes sense the proto types guaranteeing non-nil values in the store (#294) and the intentional best-effort design for RemoveExcessFeeds (#295) are things I missed during the audit. Good to know. I've updated both PRs accordingly:
The rejected changes have been removed from both branches. Let me know if anything else needs adjusting. Thank you. |
|
Can you rename the PR to reflect the changes? |
|
Hey @Thaleszh, done!
Let me know if anything else needs adjusting, thank you. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Summary
Second round of security audit fixes addressing three code defects found across the oracle and tokenfactory modules.
Changes
1. Fix NewClaim Weight parameter (x/oracle/types/ballot.go)
The
NewClaimconstructor accepted aweightparameter but incorrectly assigned thepowervalue to theWeightfield:This caused the
Weightfield to always equalPower, ignoring the intendedweightargument passed by callers.2. Propagate errors from RemoveExcessFeeds (x/oracle/keeper/keeper.go)
RemoveExcessFeedssilently dropped errors fromExchangeRate.Remove(), logging them but always returningnil. This masked cleanup failures and allowed stale exchange rates to persist without the caller knowing.The fix tracks removal errors and returns the last error while still attempting all removals:
3. Nil check in GetAuthorityMetadata (x/tokenfactory/keeper/admins.go)
GetAuthorityMetadatacalledproto.Unmarshalwith nil bytes when a denom had no authority metadata in the store.proto.Unmarshal(nil, ...)silently succeeds and returns an empty struct withAdmin="", masking the absence of the denom metadata.Added a nil check on store bytes to return an explicit error instead of silently returning empty metadata.
Testing