[C-968] feat(authz): keeper now implements permissions.ContractBlacklistListener#85
Open
kakysha wants to merge 4 commits intov0.50.x-injfrom
Open
[C-968] feat(authz): keeper now implements permissions.ContractBlacklistListener#85kakysha wants to merge 4 commits intov0.50.x-injfrom
kakysha wants to merge 4 commits intov0.50.x-injfrom
Conversation
|
@kakysha your pull request is missing a changelog! |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded a per-(grantee, msgType, granter) index and helpers, persisted and removed index entries on grant create/delete/expiry, introduced an EVM blacklist callback to enumerate and revoke affected grants (when the blacklisted address is granter or grantee), and added a PermissionsKeeper hook plus an upfront SendAuthorization permission check in Grant. Changes
Sequence DiagramsequenceDiagram
participant EVM as EVM Event
participant Keeper
participant Store as KV Store
participant Index as GrantsByGrantee Index
EVM->>Keeper: OnEnforcedRestrictionsEVMContractBlacklist(contract, user)
activate Keeper
Keeper->>Keeper: convert user -> sdk.AccAddress\nderive SendAuthorization msgTypeURL
Note over Keeper,Store: Revoke grants where user is granter
Keeper->>Store: iterate grants by granter+msgType
Store-->>Keeper: grant entries (grantee, msgType, granter)
loop per grant
Keeper->>Index: delete index key (grantee,msgType,granter)
Keeper->>Store: delete grant payload
end
Note over Keeper,Index: Revoke grants where user is grantee
Keeper->>Index: iterate index keys for (grantee=user,msgType)
Index-->>Keeper: granter addresses
loop per granter
Keeper->>Index: delete index key (user,msgType,granter)
Keeper->>Store: delete grant payload
end
deactivate Keeper
Keeper-->>EVM: return error | nil
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 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 docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
x/authz/keeper/keeper.go (1)
458-463:⚠️ Potential issue | 🟠 MajorMissing index cleanup in
DequeueAndDeleteExpiredGrants.When expired grants are deleted here, the corresponding
GrantsByGranteeindex entries are not removed. This creates orphan index entries that will causegetGrantersOfGranteeForMsgTypeto return stale data, potentially breakingOnEnforcedRestrictionsEVMContractBlacklist.🐛 Proposed fix
for _, typeURL := range queueItem.MsgTypeUrls { + // delete index entry + indexKey := grantsByGranteeAndMsgTypeAndGranterKey(grantee, typeURL, granter) + err = store.Delete(indexKey) + if err != nil { + return err + } + // delete grant err = store.Delete(grantStoreKey(grantee, granter, typeURL)) if err != nil { return err } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x/authz/keeper/keeper.go` around lines 458 - 463, DequeueAndDeleteExpiredGrants deletes expired grant entries via store.Delete(grantStoreKey(grantee, granter, typeURL)) but does not remove the corresponding GrantsByGrantee index entries, leaving orphaned indexes that make getGrantersOfGranteeForMsgType return stale data (impacting OnEnforcedRestrictionsEVMContractBlacklist). Update the deletion loop in DequeueAndDeleteExpiredGrants to also delete the index entry for each deleted grant—call the existing index key constructor (e.g., GrantsByGranteeIndexKey or grantsByGranteeKey) with the same grantee, granter and typeURL and call store.Delete on that key immediately after deleting the grant store key so both the grant and its GrantsByGrantee index are removed.
🧹 Nitpick comments (3)
x/authz/keeper/keeper.go (3)
519-521: Thecontractparameter is unused.The
contract common.Addressparameter is not used in the function body. If this is intentional for interface compliance, consider adding a comment. Otherwise, it might indicate missing functionality (e.g., logging which contract triggered the blacklist).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x/authz/keeper/keeper.go` around lines 519 - 521, The function OnEnforcedRestrictionsEVMContractBlacklist has an unused parameter contract common.Address; either use it (e.g., include contract in any relevant logs or in the enforcement action inside Keeper.OnEnforcedRestrictionsEVMContractBlacklist) or explicitly mark it as intentionally unused (add a brief comment noting interface compliance and why contract is ignored) or rename/underscore it to _contract to avoid an unused variable compiler warning; update the code paths that handle userAddr and any logging in Keeper to include contract when appropriate so the contract that triggered the blacklist is recorded.
469-495: Consider adding gas metering for iteration.Other iteration methods in this file (e.g.,
removeFromGrantQueueat line 412) consume gas per iteration. This method could iterate over many grants without charging gas, which could be exploited.♻️ Proposed fix to add gas metering
func (k Keeper) getAllGranterAuthorizations(c context.Context, granter sdk.AccAddress, msgType string) ([]sdk.AccAddress, error) { ctx := sdk.UnwrapSDKContext(c) store := k.storeService.OpenKVStore(ctx) key := grantStorePrefix(granter) iterator, err := store.Iterator(key, storetypes.PrefixEndBytes(key)) if err != nil { return nil, err } defer iterator.Close() var grantees []sdk.AccAddress for ; iterator.Valid(); iterator.Next() { + ctx.GasMeter().ConsumeGas(gasCostPerIteration, "get granter authorizations") _, grantee, authMsgType := parseGrantStoreKey(iterator.Key()) if authMsgType != msgType { continue } grantees = append(grantees, grantee) } return grantees, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x/authz/keeper/keeper.go` around lines 469 - 495, The getAllGranterAuthorizations loop currently does not charge gas and can be abused for large iterations; mirror the pattern used in removeFromGrantQueue by charging gas each iteration. Inside getAllGranterAuthorizations' for-loop (after parseGrantStoreKey or at loop start) call the same gas-consumption helper used elsewhere (or ctx.GasMeter().ConsumeGas with an appropriate sdk.Gas amount and a descriptive message like "authz:getAllGranterAuthorizations") so each iteration consumes gas; ensure you import/use the same gas accounting/utility the keeper uses and keep the placement consistent with how removeFromGrantQueue meters gas.
497-517: Same gas metering consideration applies here.Add gas consumption per iteration for consistency with other iteration methods in this file.
♻️ Proposed fix
func (k Keeper) getGrantersOfGranteeForMsgType(c context.Context, grantee sdk.AccAddress, msgType string) ([]sdk.AccAddress, error) { ctx := sdk.UnwrapSDKContext(c) store := k.storeService.OpenKVStore(ctx) key := grantsByGranteeAndMsgTypePrefix(grantee, msgType) iterator, err := store.Iterator(key, storetypes.PrefixEndBytes(key)) if err != nil { return nil, err } defer iterator.Close() var granters []sdk.AccAddress for ; iterator.Valid(); iterator.Next() { + ctx.GasMeter().ConsumeGas(gasCostPerIteration, "get granters of grantee") _, _, granter := parseIndexByGranteeAndMsgTypeKey(iterator.Key()) granters = append(granters, granter) } return granters, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x/authz/keeper/keeper.go` around lines 497 - 517, The loop in getGrantersOfGranteeForMsgType does not meter gas per iteration; add the same per-iteration gas consumption used by other iterators in this file by invoking the existing gas-metering call (e.g., the Keeper helper used elsewhere such as k.consumeGas or the ctx.GasMeter().ConsumeGas(...) pattern) at the start of the for-loop inside getGrantersOfGranteeForMsgType, using the same gas amount and error handling semantics as the other iteration helpers to ensure consistent gas accounting while parsing keys with parseIndexByGranteeAndMsgTypeKey.
🤖 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/authz/keeper/keeper.go`:
- Around line 525-531: The call to getAllGranterAuthorizations is missing an
error check so you may iterate over nil or stale grantees; after calling
getAllGranterAuthorizations(ctx, userAddr, msgTypeURL) check if err != nil and
return a wrapped error (e.g., fmt.Errorf("failed to get granter authorizations:
%w", err)) before looping, keeping the existing DeleteGrant calls
(k.DeleteGrant(ctx, grantee, userAddr, msgTypeURL)) unchanged; this ensures
failures in getAllGranterAuthorizations are propagated and not silently ignored.
- Line 12: The build fails because github.com/ethereum/go-ethereum/common (used
as common.Address in OnEnforcedRestrictionsEVMContractBlacklist) is not in
go.mod; fix by either running `go get github.com/ethereum/go-ethereum` to add
the dependency and re-run `go mod tidy`, or avoid the heavy dependency by
defining a local type alias (e.g., type EthAddress = string or type EthAddress
[20]byte) and replace uses of common.Address in the
OnEnforcedRestrictionsEVMContractBlacklist signature and body (and any helper
functions) with that alias to keep types consistent.
---
Outside diff comments:
In `@x/authz/keeper/keeper.go`:
- Around line 458-463: DequeueAndDeleteExpiredGrants deletes expired grant
entries via store.Delete(grantStoreKey(grantee, granter, typeURL)) but does not
remove the corresponding GrantsByGrantee index entries, leaving orphaned indexes
that make getGrantersOfGranteeForMsgType return stale data (impacting
OnEnforcedRestrictionsEVMContractBlacklist). Update the deletion loop in
DequeueAndDeleteExpiredGrants to also delete the index entry for each deleted
grant—call the existing index key constructor (e.g., GrantsByGranteeIndexKey or
grantsByGranteeKey) with the same grantee, granter and typeURL and call
store.Delete on that key immediately after deleting the grant store key so both
the grant and its GrantsByGrantee index are removed.
---
Nitpick comments:
In `@x/authz/keeper/keeper.go`:
- Around line 519-521: The function OnEnforcedRestrictionsEVMContractBlacklist
has an unused parameter contract common.Address; either use it (e.g., include
contract in any relevant logs or in the enforcement action inside
Keeper.OnEnforcedRestrictionsEVMContractBlacklist) or explicitly mark it as
intentionally unused (add a brief comment noting interface compliance and why
contract is ignored) or rename/underscore it to _contract to avoid an unused
variable compiler warning; update the code paths that handle userAddr and any
logging in Keeper to include contract when appropriate so the contract that
triggered the blacklist is recorded.
- Around line 469-495: The getAllGranterAuthorizations loop currently does not
charge gas and can be abused for large iterations; mirror the pattern used in
removeFromGrantQueue by charging gas each iteration. Inside
getAllGranterAuthorizations' for-loop (after parseGrantStoreKey or at loop
start) call the same gas-consumption helper used elsewhere (or
ctx.GasMeter().ConsumeGas with an appropriate sdk.Gas amount and a descriptive
message like "authz:getAllGranterAuthorizations") so each iteration consumes
gas; ensure you import/use the same gas accounting/utility the keeper uses and
keep the placement consistent with how removeFromGrantQueue meters gas.
- Around line 497-517: The loop in getGrantersOfGranteeForMsgType does not meter
gas per iteration; add the same per-iteration gas consumption used by other
iterators in this file by invoking the existing gas-metering call (e.g., the
Keeper helper used elsewhere such as k.consumeGas or the
ctx.GasMeter().ConsumeGas(...) pattern) at the start of the for-loop inside
getGrantersOfGranteeForMsgType, using the same gas amount and error handling
semantics as the other iteration helpers to ensure consistent gas accounting
while parsing keys with parseIndexByGranteeAndMsgTypeKey.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e9554aaf-85b7-4d5b-90dc-24a013cb85eb
📒 Files selected for processing (3)
x/authz/keeper/keeper.gox/authz/keeper/keeper_test.gox/authz/keeper/keys.go
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
+ added index by grantee for all the grants + tests
kakysha
force-pushed
the
C-968/usdc-remove-authz-grants
branch
from
2aac257 to
987dbb0
Compare
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/authz/keeper/keeper.go (1)
458-463:⚠️ Potential issue | 🔴 CriticalCritical:
DequeueAndDeleteExpiredGrantsdoes not delete the new grantee index.When grants expire, this function deletes only the grant store key but not the corresponding index key added by this PR. This will leave orphaned index entries, causing
getGrantersOfGranteeForMsgTypeto return stale granters that no longer have valid grants.🐛 Proposed fix
for _, typeURL := range queueItem.MsgTypeUrls { + // delete index + indexKey := grantsByGranteeAndMsgTypeAndGranterKey(grantee, typeURL, granter) + err = store.Delete(indexKey) + if err != nil { + return err + } + err = store.Delete(grantStoreKey(grantee, granter, typeURL)) if err != nil { return err } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x/authz/keeper/keeper.go` around lines 458 - 463, DequeueAndDeleteExpiredGrants currently deletes only the grant store entry (using grantStoreKey) for each expired grant but must also remove the corresponding grantee index entry so getGrantersOfGranteeForMsgType won't return stale granters; update the loop that iterates over queueItem.MsgTypeUrls to call store.Delete for the grantee index key (e.g., granteeIndexKey or the actual index key helper added in this PR, passing grantee, typeURL and granter as required) immediately after deleting grantStoreKey, and propagate/handle any error exactly like the existing grant delete (return the error if delete fails).
♻️ Duplicate comments (1)
x/authz/keeper/keeper.go (1)
12-12:⚠️ Potential issue | 🔴 CriticalBuild is still failing due to missing
go-ethereumdependency.The pipeline continues to fail with "cannot find module providing package github.com/ethereum/go-ethereum/common". Please verify that the fix in commit 987dbb0 was properly applied and that
go.modincludes the dependency.,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x/authz/keeper/keeper.go` at line 12, The build fails because keeper.go imports "github.com/ethereum/go-ethereum/common" but go.mod does not include the go-ethereum module; update the module dependencies by running a module fetch and tidy (e.g., run `go get github.com/ethereum/go-ethereum@latest` or add the appropriate version to go.mod) and then `go mod tidy` to ensure github.com/ethereum/go-ethereum is recorded; verify the import in keeper.go remains `github.com/ethereum/go-ethereum/common` and confirm the fix from commit 987dbb0 is present in the branch.
🧹 Nitpick comments (1)
x/authz/keeper/keeper.go (1)
468-495: Consider adding gas metering for iteration consistency.Other iteration functions in this file (e.g.,
removeFromGrantQueueat line 412) consume gas per iteration usinggasCostPerIteration. While this function is called from a blacklist callback context, adding gas metering would maintain consistency and provide protection if the function is reused elsewhere.♻️ Optional: Add gas metering
for ; iterator.Valid(); iterator.Next() { + ctx.GasMeter().ConsumeGas(gasCostPerIteration, "get granter authorizations") _, grantee, authMsgType := parseGrantStoreKey(iterator.Key())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x/authz/keeper/keeper.go` around lines 468 - 495, The getAllGranterAuthorizations function lacks per-iteration gas metering; update it to consume gas inside the iteration similar to removeFromGrantQueue: after unwrapping ctx (ctx := sdk.UnwrapSDKContext(c)) obtain the gas meter via ctx.GasMeter() and, inside the for ; iterator.Valid(); iterator.Next() loop, call gasMeter.ConsumeGas(gasCostPerIteration, "getAllGranterAuthorizations iteration") (or the same message used in removeFromGrantQueue) before parsing the key, so each iteration charges gas and protects reuse of this function outside blacklist callbacks.
🤖 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/authz/keeper/keeper.go`:
- Around line 519-549: The function OnEnforcedRestrictionsEVMContractBlacklist
currently leaves the contract parameter unused and only revokes
SendAuthorization; mark the unused parameter (add "_ = contract") and update the
revocation logic to remove all grant types: instead of using the specific
banktypes.SendAuthorization{}.MsgTypeURL(), call the existing grant-enumeration
helpers to fetch every authorization where the blacklisted user is a granter and
every authorization where they are a grantee (or add a small helper that lists
grants without filtering by msg type), then iterate and call DeleteGrant for
each returned (use getAllGranterAuthorizations and the counterpart that lists
granters for a grantee, or add equivalents if needed) so all authorization types
are deleted, referencing OnEnforcedRestrictionsEVMContractBlacklist,
getAllGranterAuthorizations, getGrantersOfGranteeForMsgType (or its no-msg-type
variant), and DeleteGrant.
---
Outside diff comments:
In `@x/authz/keeper/keeper.go`:
- Around line 458-463: DequeueAndDeleteExpiredGrants currently deletes only the
grant store entry (using grantStoreKey) for each expired grant but must also
remove the corresponding grantee index entry so getGrantersOfGranteeForMsgType
won't return stale granters; update the loop that iterates over
queueItem.MsgTypeUrls to call store.Delete for the grantee index key (e.g.,
granteeIndexKey or the actual index key helper added in this PR, passing
grantee, typeURL and granter as required) immediately after deleting
grantStoreKey, and propagate/handle any error exactly like the existing grant
delete (return the error if delete fails).
---
Duplicate comments:
In `@x/authz/keeper/keeper.go`:
- Line 12: The build fails because keeper.go imports
"github.com/ethereum/go-ethereum/common" but go.mod does not include the
go-ethereum module; update the module dependencies by running a module fetch and
tidy (e.g., run `go get github.com/ethereum/go-ethereum@latest` or add the
appropriate version to go.mod) and then `go mod tidy` to ensure
github.com/ethereum/go-ethereum is recorded; verify the import in keeper.go
remains `github.com/ethereum/go-ethereum/common` and confirm the fix from commit
987dbb0 is present in the branch.
---
Nitpick comments:
In `@x/authz/keeper/keeper.go`:
- Around line 468-495: The getAllGranterAuthorizations function lacks
per-iteration gas metering; update it to consume gas inside the iteration
similar to removeFromGrantQueue: after unwrapping ctx (ctx :=
sdk.UnwrapSDKContext(c)) obtain the gas meter via ctx.GasMeter() and, inside the
for ; iterator.Valid(); iterator.Next() loop, call
gasMeter.ConsumeGas(gasCostPerIteration, "getAllGranterAuthorizations
iteration") (or the same message used in removeFromGrantQueue) before parsing
the key, so each iteration charges gas and protects reuse of this function
outside blacklist callbacks.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 47ae802b-ffe3-4eda-b984-8b4bab556490
📒 Files selected for processing (3)
x/authz/keeper/keeper.gox/authz/keeper/keeper_test.gox/authz/keeper/keys.go
🚧 Files skipped from review as they are similar to previous changes (2)
- x/authz/keeper/keys.go
- x/authz/keeper/keeper_test.go
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@go.mod`:
- Line 32: The go-ethereum dependency in go.mod is pinned to the vulnerable
version "github.com/ethereum/go-ethereum v1.15.7"; update that module entry to a
patched release (preferably v1.17.2, or at minimum v1.16.8) to address the
listed HIGH severity vulnerabilities, then run go mod tidy to update the go.sum
and any transitive deps and run your test suite/build to verify compatibility;
check for and remove any local replace directives that could force the old
version if present.
In `@simapp/go.mod`:
- Line 26: The go-ethereum dependency in simapp's go.mod is pinned to the
vulnerable version "github.com/ethereum/go-ethereum v1.15.7"; update that module
to v1.17.2 (or at least v1.16.8+) to pull in fixes for the listed GHSA issues,
then run module housekeeping. Concretely: replace the version token
"github.com/ethereum/go-ethereum v1.15.7" with "github.com/ethereum/go-ethereum
v1.17.2" (or run `go get github.com/ethereum/go-ethereum@v1.17.2`), run `go mod
tidy` to update go.sum, and then rebuild/tests to ensure no breakages.
In `@tests/go.mod`:
- Line 25: The dependency line for github.com/ethereum/go-ethereum in
tests/go.mod is pinned to v1.15.7 and must be updated to v1.17.2 to address
multiple HIGH severity vulnerabilities; edit the version token for the module
name "github.com/ethereum/go-ethereum" to v1.17.2, then run dependency
maintenance (e.g., go mod tidy / go get github.com/ethereum/go-ethereum@v1.17.2)
to refresh the module graph and vendor artifacts, and finally run the test
suite/CI to ensure no breaking changes affect code that imports that package.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 15153c10-2c69-4700-b55a-7a4bef7ca5ce
⛔ Files ignored due to path filters (3)
go.sumis excluded by!**/*.sumsimapp/go.sumis excluded by!**/*.sumtests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
go.modsimapp/go.modtests/go.mod
There was a problem hiding this comment.
🧹 Nitpick comments (2)
x/authz/keeper/keeper.go (2)
473-499: Consider adding gas metering for iteration consistency.This function iterates over potentially unbounded grant entries without consuming gas, unlike
removeFromGrantQueuewhich chargesgasCostPerIterationper loop. If this function is called during transaction processing (as it is viaOnEnforcedRestrictionsEVMContractBlacklist), the lack of gas metering could allow expensive iterations without proper gas accounting.♻️ Proposed fix to add gas metering
func (k Keeper) getAllGranterAuthorizations(c context.Context, granter sdk.AccAddress, msgType string) ([]sdk.AccAddress, error) { ctx := sdk.UnwrapSDKContext(c) store := k.storeService.OpenKVStore(ctx) key := grantStorePrefix(granter) iterator, err := store.Iterator(key, storetypes.PrefixEndBytes(key)) if err != nil { return nil, err } defer iterator.Close() var grantees []sdk.AccAddress for ; iterator.Valid(); iterator.Next() { + ctx.GasMeter().ConsumeGas(gasCostPerIteration, "get granter authorizations") _, grantee, authMsgType := parseGrantStoreKey(iterator.Key()) if authMsgType != msgType { continue } grantees = append(grantees, grantee) } return grantees, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x/authz/keeper/keeper.go` around lines 473 - 499, The getAllGranterAuthorizations loop lacks gas metering; update getAllGranterAuthorizations to consume gas on each iterator step (same pattern as removeFromGrantQueue) by retrieving the SDK context's GasMeter and calling ConsumeGas (or the existing gas consumption helper) with the gasCostPerIteration value inside the for loop before processing each key; reference getAllGranterAuthorizations, the iterator loop, and the existing removeFromGrantQueue gas consumption logic to mirror the implementation and ensure consistent gas accounting.
501-521: Same gas metering concern applies here.Similar to
getAllGranterAuthorizations, this function iterates without gas metering. Consider addingctx.GasMeter().ConsumeGas(gasCostPerIteration, "get grantee authorizations")in the loop for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x/authz/keeper/keeper.go` around lines 501 - 521, The loop in getGrantersOfGranteeForMsgType lacks gas metering similar to getAllGranterAuthorizations; inside the for loop that iterates over iterator.Valid()/iterator.Next() in the getGrantersOfGranteeForMsgType function, call ctx.GasMeter().ConsumeGas(gasCostPerIteration, "get grantee authorizations") on each iteration before parsing the key (where parseIndexByGranteeAndMsgTypeKey is used) to ensure consistent gas accounting; use the same gasCostPerIteration value/name used elsewhere to keep behavior consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@x/authz/keeper/keeper.go`:
- Around line 473-499: The getAllGranterAuthorizations loop lacks gas metering;
update getAllGranterAuthorizations to consume gas on each iterator step (same
pattern as removeFromGrantQueue) by retrieving the SDK context's GasMeter and
calling ConsumeGas (or the existing gas consumption helper) with the
gasCostPerIteration value inside the for loop before processing each key;
reference getAllGranterAuthorizations, the iterator loop, and the existing
removeFromGrantQueue gas consumption logic to mirror the implementation and
ensure consistent gas accounting.
- Around line 501-521: The loop in getGrantersOfGranteeForMsgType lacks gas
metering similar to getAllGranterAuthorizations; inside the for loop that
iterates over iterator.Valid()/iterator.Next() in the
getGrantersOfGranteeForMsgType function, call
ctx.GasMeter().ConsumeGas(gasCostPerIteration, "get grantee authorizations") on
each iteration before parsing the key (where parseIndexByGranteeAndMsgTypeKey is
used) to ensure consistent gas accounting; use the same gasCostPerIteration
value/name used elsewhere to keep behavior consistent.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
x/authz/keeper/keeper.go (1)
528-557:⚠️ Potential issue | 🟠 MajorAlign blacklist revocation with denom-scoped enforcement.
x/authz/keeper/msg_server.go, Lines 61-71, only treats enforced denoms as special, but this callback deletes everySendAuthorizationfor the account without checkingcontractor the grant'sSpendLimit. A blacklist event for one enforced token will currently wipe unrelated or non-enforced send grants too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x/authz/keeper/keeper.go` around lines 528 - 557, The callback OnEnforcedRestrictionsEVMContractBlacklist currently deletes every SendAuthorization for the account; change it to only remove grants that actually allow spending of the enforced token (i.e., check the grant's authorization SpendLimit includes the denom associated with the provided contract). Specifically: when iterating grantees from getAllGranterAuthorizations and granters from getGrantersOfGranteeForMsgType, load each grant's authorization (ensure it is a banktypes.SendAuthorization), inspect its SpendLimit for the enforced denom derived from the contract parameter, and only call DeleteGrant for grants whose SpendLimit contains that denom (leave unrelated SendAuthorizations intact). Use the existing symbols OnEnforcedRestrictionsEVMContractBlacklist, banktypes.SendAuthorization, getAllGranterAuthorizations, getGrantersOfGranteeForMsgType and DeleteGrant to locate and implement the filter.
🤖 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/authz/keeper/keeper.go`:
- Around line 60-62: SetPermissionsKeeper is unused outside tests and leaves
Keeper.permissionsKeeper nil (causing grant-time checks in msg_server.go to be
skipped); either wire it from app initialization or remove the setter and
associated nil-conditional behavior. To fix, either (A) add a PermissionsKeeper
parameter to the Keeper constructor (or call
keeper.SetPermissionsKeeper(app.PermissionsKeeper) in your app/module wiring)
and set k.permissionsKeeper there so msg_server.go's grant-time checks run, or
(B) delete SetPermissionsKeeper, remove the nil checks in
x/authz/keeper/msg_server.go that skip restrictions when permissionsKeeper ==
nil, and ensure tests are updated to inject a mock via the constructor if
needed. Ensure you update references to SetPermissionsKeeper and adjust tests
accordingly.
- Around line 228-233: The migration fails to backfill the GrantsByGrantee
reverse index used by getGrantersOfGranteeForMsgType(), so pre-upgrade grants
won't be discoverable; update the v2 migration (x/authz/migrations/v2/store.go)
to iterate all existing grants (the same source used by SaveGrant/SaveGrantGrant
or SaveGrant) and for each grant create and Set the index key using
grantsByGranteeAndMsgTypeAndGranterKey(grantee, msgType, granter) (the same key
logic used in SaveGrant) so the GrantsByGrantee index is populated for existing
records and getGrantersOfGranteeForMsgType() will work for pre-upgrade grants.
---
Outside diff comments:
In `@x/authz/keeper/keeper.go`:
- Around line 528-557: The callback OnEnforcedRestrictionsEVMContractBlacklist
currently deletes every SendAuthorization for the account; change it to only
remove grants that actually allow spending of the enforced token (i.e., check
the grant's authorization SpendLimit includes the denom associated with the
provided contract). Specifically: when iterating grantees from
getAllGranterAuthorizations and granters from getGrantersOfGranteeForMsgType,
load each grant's authorization (ensure it is a banktypes.SendAuthorization),
inspect its SpendLimit for the enforced denom derived from the contract
parameter, and only call DeleteGrant for grants whose SpendLimit contains that
denom (leave unrelated SendAuthorizations intact). Use the existing symbols
OnEnforcedRestrictionsEVMContractBlacklist, banktypes.SendAuthorization,
getAllGranterAuthorizations, getGrantersOfGranteeForMsgType and DeleteGrant to
locate and implement the filter.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b82b0c2c-da93-4a9c-810a-302c9db6bea1
📒 Files selected for processing (4)
x/authz/expected_keepers.gox/authz/keeper/keeper.gox/authz/keeper/keeper_test.gox/authz/keeper/msg_server.go
🚧 Files skipped from review as they are similar to previous changes (1)
- x/authz/keeper/keeper_test.go
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.
Summary by CodeRabbit
New Features
Bug Fixes
Tests