Conversation
📝 WalkthroughWalkthroughRefactors listener goroutine/channel closing, fixes utility panics and slice allocation, changes JWS token keeper APIs and error/time handling, corrects credit accumulation, simplifies ACP ownership logic, and adds many ACP unit & simulation tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (12)
x/acp/types/errors_test.go (1)
36-45: Strengthen constructor/wrap tests to assert semantics, not just non-nil.Current checks can pass even if message composition or cause propagation breaks. Consider asserting message content and
errors.Is/errors.Unwrapbehavior.Suggested test hardening
package types import ( + "errors" "fmt" "testing" @@ func TestErrorConstructorAliases(t *testing.T) { err := New("test error", ErrorType_INTERNAL) require.Error(t, err) + require.Contains(t, err.Error(), "test error") wrapped := Wrap("wrapped", err) require.Error(t, wrapped) + require.Contains(t, wrapped.Error(), "wrapped") + require.True(t, errors.Is(wrapped, err)) - withCause := NewWithCause("msg", fmt.Errorf("cause"), ErrorType_BAD_INPUT) + cause := fmt.Errorf("cause") + withCause := NewWithCause("msg", cause, ErrorType_BAD_INPUT) require.Error(t, withCause) + require.Contains(t, withCause.Error(), "msg") + require.True(t, errors.Is(withCause, cause)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x/acp/types/errors_test.go` around lines 36 - 45, Update TestErrorConstructorAliases to assert semantics not just non-nil: verify that errors created by New("test error", ErrorType_INTERNAL), Wrap("wrapped", err) and NewWithCause("msg", fmt.Errorf("cause"), ErrorType_BAD_INPUT) contain the expected messages (e.g., check strings.Contains on Error() or exact text where appropriate) and that cause propagation works using errors.Is/errors.Unwrap (e.g., errors.Is(wrapped, err) or errors.Unwrap(wrapped) == err and errors.Unwrap(withCause) returns the fmt.Errorf("cause") or is identifiable via errors.Is); use the New, Wrap and NewWithCause identifiers and the TestErrorConstructorAliases test to add these assertions.x/acp/types/codec_test.go (1)
10-15: Strengthen this test with a behavioral registration assertionLine 12 currently only checks panic safety; it will still pass if
RegisterInterfacesbecomes a no-op. Please add one assertion that verifies a known ACP concrete type can be encoded/decoded via the registry after registration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x/acp/types/codec_test.go` around lines 10 - 15, TestRegisterInterfaces only checks for panics; update it to also assert behavioral registration by creating an instance of a known exported ACP concrete type from the x/acp/types package, registering the registry via RegisterInterfaces(registry), wrapping the instance into a codectypes.Any (or using the codec's marshaler) and then decoding/unwrapping it back — assert the resulting type/value matches the original. Use the existing registry variable and RegisterInterfaces function to perform the registration and verify encode->decode roundtrip for that concrete type.x/acp/did/did_test.go (1)
9-15: Consider asserting semantic DID validity, not just substring presence.Current checks can pass on malformed strings that still contain
"did:key:". AddingIsValidDID(did)would tighten coverage.Proposed test strengthening
func TestProduceDID(t *testing.T) { did, signer, err := ProduceDID() require.NoError(t, err) require.NotEmpty(t, did) require.NotNil(t, signer) require.Contains(t, did, "did:key:") + require.NoError(t, IsValidDID(did)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x/acp/did/did_test.go` around lines 9 - 15, The test TestProduceDID currently only checks for substring "did:key:" which can pass for malformed values; update the test to assert semantic validity by calling the DID validation helper (e.g., IsValidDID) on the value returned by ProduceDID and require it is true (use require.True(t, IsValidDID(did)) or equivalent), keeping the existing checks for non-empty and signer presence; ensure you reference ProduceDID and IsValidDID so the correct validation helper is used and any necessary import is added.x/acp/types/mapper_test.go (2)
65-80: Round-trip test should also validateCreationTs.This test currently checks only
OwnerDidandTxSigner; a timestamp mapping regression could slip through unnoticed.Proposed assertion add-on
func TestExtractRecordMetadata(t *testing.T) { ctx := makeTestContext(t) original, err := BuildRecordMetadata(ctx, "did:example:actor", "cosmos1creator") require.NoError(t, err) blob, err := original.Marshal() require.NoError(t, err) coreMd := &coretypes.RecordMetadata{ Supplied: &coretypes.SuppliedMetadata{Blob: blob}, } extracted, err := ExtractRecordMetadata(coreMd) require.NoError(t, err) require.Equal(t, original.OwnerDid, extracted.OwnerDid) require.Equal(t, original.TxSigner, extracted.TxSigner) + require.NotNil(t, extracted.CreationTs) + require.Equal(t, original.CreationTs.BlockHeight, extracted.CreationTs.BlockHeight) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x/acp/types/mapper_test.go` around lines 65 - 80, The test TestExtractRecordMetadata currently asserts only OwnerDid and TxSigner; add an assertion to also compare the CreationTs from the original metadata and the extracted metadata to guard against timestamp mapping regressions. Locate TestExtractRecordMetadata (and the variables original from BuildRecordMetadata and extracted from ExtractRecordMetadata) and add a require.Equal comparing original.CreationTs (or the actual timestamp field on original) to extracted.CreationTs so the round-trip validates the creation timestamp as well.
53-63: StrengthenWithTimetest to assert the provided timestamp is actually used.Right now this can pass even if
BuildACPSuppliedMetadataWithTimeignorests. Assert decoded metadata has block height50(fromts), not just non-empty blob.Proposed test hardening
func TestBuildACPSuppliedMetadataWithTime(t *testing.T) { ctx := makeTestContext(t) protoTs, err := prototypes.TimestampProto(time.Date(2024, time.March, 1, 0, 0, 0, 0, time.UTC)) require.NoError(t, err) ts := NewTimestamp(protoTs, 50) sm, err := BuildACPSuppliedMetadataWithTime(ctx, ts, "did:example:actor", "cosmos1creator") require.NoError(t, err) require.NotNil(t, sm) require.NotEmpty(t, sm.Blob) + + extracted, err := ExtractRecordMetadata(&coretypes.RecordMetadata{ + Supplied: &coretypes.SuppliedMetadata{Blob: sm.Blob}, + }) + require.NoError(t, err) + require.NotNil(t, extracted.CreationTs) + require.Equal(t, uint64(50), extracted.CreationTs.BlockHeight) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x/acp/types/mapper_test.go` around lines 53 - 63, The test currently only checks sm.Blob is non-empty but doesn't verify the provided timestamp was used; update TestBuildACPSuppliedMetadataWithTime to decode/unmarshal sm.Blob into the same SuppliedMetadata/protobuf type produced by BuildACPSuppliedMetadataWithTime and assert the decoded metadata's block height equals 50 (the value set by NewTimestamp(ts, 50)); use the existing ts and sm.Blob symbols to locate the values to decode and add an assertion that the timestamp-derived block height is exactly 50.x/acp/types/constants_test.go (1)
9-12: Redundant type cast.Both
43200as a typed literal andDefaultPolicyCommandMaxExpirationDeltacan be compared directly since the constant is alreadyuint64.♻️ Suggested fix
func TestDefaultPolicyCommandMaxExpirationDelta(t *testing.T) { // 12 hours in seconds - require.Equal(t, uint64(43200), uint64(DefaultPolicyCommandMaxExpirationDelta)) + require.Equal(t, uint64(43200), DefaultPolicyCommandMaxExpirationDelta) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x/acp/types/constants_test.go` around lines 9 - 12, The test TestDefaultPolicyCommandMaxExpirationDelta uses redundant casts; remove the unnecessary uint64(...) wrappers and compare the constant DefaultPolicyCommandMaxExpirationDelta directly to the integer literal (e.g., require.Equal(t, uint64(43200), DefaultPolicyCommandMaxExpirationDelta) should be simplified by dropping the cast on the constant or the literal so the types match without double-casting and use DefaultPolicyCommandMaxExpirationDelta as the uint64 value in the assertion).x/acp/types/params_test.go (1)
17-21: Redundant type cast on line 19.
DefaultPolicyCommandMaxExpirationDeltais alreadyuint64, making the cast unnecessary.♻️ Suggested fix
func TestDefaultParams(t *testing.T) { p := DefaultParams() - require.Equal(t, uint64(DefaultPolicyCommandMaxExpirationDelta), p.PolicyCommandMaxExpirationDelta) + require.Equal(t, DefaultPolicyCommandMaxExpirationDelta, p.PolicyCommandMaxExpirationDelta) require.NotNil(t, p.RegistrationsCommitmentValidity) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x/acp/types/params_test.go` around lines 17 - 21, Remove the redundant uint64 cast in TestDefaultParams: update the assertion that compares DefaultPolicyCommandMaxExpirationDelta to p.PolicyCommandMaxExpirationDelta in the TestDefaultParams function so it passes DefaultPolicyCommandMaxExpirationDelta directly (no explicit type conversion), leaving the equality check between DefaultPolicyCommandMaxExpirationDelta and p.PolicyCommandMaxExpirationDelta unchanged.x/acp/stores/marshaler_test.go (2)
21-23: Consider asserting additional fields for stronger round-trip validation.Currently only
PolicyCommandMaxExpirationDeltais verified. AssertingRegistrationsCommitmentValiditywould strengthen confidence in the full round-trip.♻️ Suggested enhancement
decoded, err := marshaler.Unmarshal(bytes) require.NoError(t, err) require.Equal(t, original.PolicyCommandMaxExpirationDelta, decoded.PolicyCommandMaxExpirationDelta) + require.Equal(t, original.RegistrationsCommitmentValidity, decoded.RegistrationsCommitmentValidity)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x/acp/stores/marshaler_test.go` around lines 21 - 23, Add an assertion to the round-trip test to verify RegistrationsCommitmentValidity as well: after unmarshaling (decoded, err := marshaler.Unmarshal(bytes)) and the existing assertion for PolicyCommandMaxExpirationDelta, add require.Equal(t, original.RegistrationsCommitmentValidity, decoded.RegistrationsCommitmentValidity) so the test verifies both fields from the original struct.
15-17: Consider simplifying the double-pointer pattern.The current approach creates a confusing double-pointer (
**types.Params) to satisfy theMarshal(*T)signature whereT = *types.Params. This works but obscures intent.♻️ Suggested simplification
- original := types.DefaultParams() - origPtr := &original - bytes, err := marshaler.Marshal(&origPtr) + original := types.DefaultParams() + bytes, err := marshaler.Marshal(&original)This requires changing the factory to return
types.Params(non-pointer) instead:- marshaler := NewGogoProtoMarshaler(func() *types.Params { - return &types.Params{} + marshaler := NewGogoProtoMarshaler(func() types.Params { + return types.Params{} })However, if the production code uses
*types.Paramsas the generic type, the current pattern is correct. Verify the factory pattern matches production usage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x/acp/stores/marshaler_test.go` around lines 15 - 17, The test is creating a confusing double-pointer by doing original := types.DefaultParams(); origPtr := &original; marshaler.Marshal(&origPtr) — simplify by making the factory and call consistent: change types.DefaultParams to return a non-pointer types.Params and then call marshaler.Marshal(&original) (so you pass *types.Params), or if production uses *types.Params instead, change the factory to return *types.Params and call marshaler.Marshal(original) directly; update usages of DefaultParams, origPtr and the marshaler.Marshal call to match the chosen single-pointer convention.x/acp/types/time_test.go (1)
95-97: Assert the panic value to lock the contract.
require.Panicspasses for any panic. Since this path is intentionallypanic("invalid duration")inx/acp/types/time.go:62, userequire.PanicsWithValueto make this test stricter.Proposed test hardening
- require.Panics(t, func() { + require.PanicsWithValue(t, "invalid duration", func() { ts.IsAfter(duration, now) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x/acp/types/time_test.go` around lines 95 - 97, The test currently uses require.Panics for ts.IsAfter which accepts any panic; change it to assert the exact panic value by using require.PanicsWithValue and the literal panic string "invalid duration" so the test locks the contract of IsAfter (the panic raised in the IsAfter implementation).x/acp/types/commitment_test.go (1)
53-61: Add the exact block boundary case (150) for completeness.Given
CreationTs.BlockHeight=100and validity50, asserting Line 53/58 neighbors is good; adding150explicitly makes the strict “after” behavior unambiguous.Suggested boundary assertion
now := NewTimestamp(nil, 160) expired, err := commitment.IsExpiredAgainst(now) require.NoError(t, err) require.True(t, expired) + nowBoundary := NewTimestamp(nil, 150) + expiredBoundary, err := commitment.IsExpiredAgainst(nowBoundary) + require.NoError(t, err) + require.False(t, expiredBoundary) + now2 := NewTimestamp(nil, 140) expired2, err := commitment.IsExpiredAgainst(now2) require.NoError(t, err) require.False(t, expired2)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x/acp/types/commitment_test.go` around lines 53 - 61, Add an explicit boundary test using NewTimestamp(nil, 150) against commitment.IsExpiredAgainst to verify the strict "after" semantics (CreationTs.BlockHeight=100 with validity 50); call commitment.IsExpiredAgainst(nowBoundary) and assert no error and that expired is false so the case at block 150 is explicitly covered.x/acp/capability/manager_test.go (1)
102-107: Comment acknowledges incorrect behavior but test still passes.The comment at lines 104-105 states validation "passes, but for the wrong reason." Consider adding a
TODOor linking to an issue tracking theisOwnedByAcpModulelogic fix so this doesn't get overlooked.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x/acp/capability/manager_test.go` around lines 102 - 107, Add a TODO note (or an issue link) next to the existing comment in the test that calls otherMgr.Validate so the incorrect behavior of isOwnedByAcpModule doesn't get forgotten; update the comment around the validation block (the lines containing otherMgr.Validate(ctx, cap) and the succeeding comment about Filter removing "acp") to include "TODO: fix isOwnedByAcpModule logic" and optionally reference the issue ID or URL tracking the bug so maintainers can find and address the root cause later.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@utils/span.go`:
- Around line 69-73: GetMsgSpan's safe type assertion can return nil but
FinalizeSpan calls GetMsgSpan(ctx).ToEvent() without checking for nil; update
FinalizeSpan to call GetMsgSpan(ctx), store the result in a local (e.g., span :=
GetMsgSpan(ctx)), check if span == nil and return early (or log a warning)
before invoking span.ToEvent(), so ToEvent() is only called on a non-nil
*MsgSpan; reference GetMsgSpan, FinalizeSpan, ToEvent, MsgSpan and spanCtxKey
when locating the code.
In `@x/acp/capability/manager_test.go`:
- Around line 137-151: The test TestIsOwnedByAcpModuleInvertedLogic exposes
inverted logic in isOwnedByAcpModule: instead of filtering out "acp" and
treating an empty filtered list as false, change isOwnedByAcpModule (manager.go)
to directly check whether "acp" is present in the capability owners (return true
when "acp" is an owner) or otherwise implement explicit semantics consistent
with its name; then update the test TestIsOwnedByAcpModuleInvertedLogic to
expect require.NoError(t, err) from acpMgr.Validate (or remove the test and add
a TODO/FIXME comment if you prefer preserving the original failing-spec
behavior). Ensure you update references to isOwnedByAcpModule and Validate so
their behavior and tests remain consistent.
- Line 21: The cap memory store is created as a KV store but must be an
in-memory store: replace the capMemStoreKey :=
storetypes.NewKVStoreKey("capkeepermem") usage with
storetypes.NewMemoryStoreKey("capkeepermem") and ensure any mounting of that key
uses storetypes.StoreTypeMemory (not StoreTypeDB) so the CapabilityKeeper is
constructed with a memory store key; update references to capMemStoreKey and the
MountStore/MountStoreWithDB calls accordingly.
In `@x/acp/did/types_test.go`:
- Around line 43-46: The test currently locks in implicit panic behavior from a
nil deref; change the contract and implementation so DIDFromPubKey validates
input and returns an error instead of panicking: add a nil guard at the start of
DIDFromPubKey (if pk == nil { return "", fmt.Errorf("account public key is nil")
}) and update its signature/return handling as needed, then update
TestDIDFromPubKeyNilPanics to assert an error/expected return value from
DIDFromPubKey(nil) (and update any callers such as IssueDID to handle the error)
so nil pubkeys are explicitly handled rather than relying on a panic.
In `@x/hub/keeper/jws_token.go`:
- Around line 106-109: GetJWSToken currently conflates "not found" and
"unmarshal/decoding" failures by returning found=false for both; update the API
to surface decode errors and change callers to wrap them correctly: change
GetJWSToken signature to return (types.JWSToken, bool, error) (preserve existing
behavior for the not-found path by returning (zero, false, nil) and return
(zero, false, err) on unmarshal/decode failures), then in callers (places now
doing record, found := GetJWSToken(...)) handle three cases: if err != nil ->
return errorsmod.Wrap(err, "decoding JWSToken" or similar), else if !found ->
return errorsmod.Wrap(types.ErrJWSTokenNotFound, tokenHash), else proceed with
record; apply the same change to all call sites referenced (the blocks currently
around GetJWSToken usage that return ErrJWSTokenNotFound).
- Around line 305-306: The expiration check in the token creation uses
expiresAt.Before(sdk.UnwrapSDKContext(ctx).BlockTime()) which allows expiresAt
equal to the current block time; update the condition to also reject equality by
using a non-after test (e.g.
!expiresAt.After(sdk.UnwrapSDKContext(ctx).BlockTime())) or combine Before ||
Equal so that if !expiresAt.IsZero() && !expiresAt.After(blockTime) you return
errorsmod.Wrap(types.ErrJWSTokenExpired, "cannot create token with expiration
time in the past"); adjust the condition where expiresAt and
sdk.UnwrapSDKContext(ctx).BlockTime() are referenced (the token creation branch)
accordingly.
---
Nitpick comments:
In `@x/acp/capability/manager_test.go`:
- Around line 102-107: Add a TODO note (or an issue link) next to the existing
comment in the test that calls otherMgr.Validate so the incorrect behavior of
isOwnedByAcpModule doesn't get forgotten; update the comment around the
validation block (the lines containing otherMgr.Validate(ctx, cap) and the
succeeding comment about Filter removing "acp") to include "TODO: fix
isOwnedByAcpModule logic" and optionally reference the issue ID or URL tracking
the bug so maintainers can find and address the root cause later.
In `@x/acp/did/did_test.go`:
- Around line 9-15: The test TestProduceDID currently only checks for substring
"did:key:" which can pass for malformed values; update the test to assert
semantic validity by calling the DID validation helper (e.g., IsValidDID) on the
value returned by ProduceDID and require it is true (use require.True(t,
IsValidDID(did)) or equivalent), keeping the existing checks for non-empty and
signer presence; ensure you reference ProduceDID and IsValidDID so the correct
validation helper is used and any necessary import is added.
In `@x/acp/stores/marshaler_test.go`:
- Around line 21-23: Add an assertion to the round-trip test to verify
RegistrationsCommitmentValidity as well: after unmarshaling (decoded, err :=
marshaler.Unmarshal(bytes)) and the existing assertion for
PolicyCommandMaxExpirationDelta, add require.Equal(t,
original.RegistrationsCommitmentValidity,
decoded.RegistrationsCommitmentValidity) so the test verifies both fields from
the original struct.
- Around line 15-17: The test is creating a confusing double-pointer by doing
original := types.DefaultParams(); origPtr := &original;
marshaler.Marshal(&origPtr) — simplify by making the factory and call
consistent: change types.DefaultParams to return a non-pointer types.Params and
then call marshaler.Marshal(&original) (so you pass *types.Params), or if
production uses *types.Params instead, change the factory to return
*types.Params and call marshaler.Marshal(original) directly; update usages of
DefaultParams, origPtr and the marshaler.Marshal call to match the chosen
single-pointer convention.
In `@x/acp/types/codec_test.go`:
- Around line 10-15: TestRegisterInterfaces only checks for panics; update it to
also assert behavioral registration by creating an instance of a known exported
ACP concrete type from the x/acp/types package, registering the registry via
RegisterInterfaces(registry), wrapping the instance into a codectypes.Any (or
using the codec's marshaler) and then decoding/unwrapping it back — assert the
resulting type/value matches the original. Use the existing registry variable
and RegisterInterfaces function to perform the registration and verify
encode->decode roundtrip for that concrete type.
In `@x/acp/types/commitment_test.go`:
- Around line 53-61: Add an explicit boundary test using NewTimestamp(nil, 150)
against commitment.IsExpiredAgainst to verify the strict "after" semantics
(CreationTs.BlockHeight=100 with validity 50); call
commitment.IsExpiredAgainst(nowBoundary) and assert no error and that expired is
false so the case at block 150 is explicitly covered.
In `@x/acp/types/constants_test.go`:
- Around line 9-12: The test TestDefaultPolicyCommandMaxExpirationDelta uses
redundant casts; remove the unnecessary uint64(...) wrappers and compare the
constant DefaultPolicyCommandMaxExpirationDelta directly to the integer literal
(e.g., require.Equal(t, uint64(43200), DefaultPolicyCommandMaxExpirationDelta)
should be simplified by dropping the cast on the constant or the literal so the
types match without double-casting and use
DefaultPolicyCommandMaxExpirationDelta as the uint64 value in the assertion).
In `@x/acp/types/errors_test.go`:
- Around line 36-45: Update TestErrorConstructorAliases to assert semantics not
just non-nil: verify that errors created by New("test error",
ErrorType_INTERNAL), Wrap("wrapped", err) and NewWithCause("msg",
fmt.Errorf("cause"), ErrorType_BAD_INPUT) contain the expected messages (e.g.,
check strings.Contains on Error() or exact text where appropriate) and that
cause propagation works using errors.Is/errors.Unwrap (e.g., errors.Is(wrapped,
err) or errors.Unwrap(wrapped) == err and errors.Unwrap(withCause) returns the
fmt.Errorf("cause") or is identifiable via errors.Is); use the New, Wrap and
NewWithCause identifiers and the TestErrorConstructorAliases test to add these
assertions.
In `@x/acp/types/mapper_test.go`:
- Around line 65-80: The test TestExtractRecordMetadata currently asserts only
OwnerDid and TxSigner; add an assertion to also compare the CreationTs from the
original metadata and the extracted metadata to guard against timestamp mapping
regressions. Locate TestExtractRecordMetadata (and the variables original from
BuildRecordMetadata and extracted from ExtractRecordMetadata) and add a
require.Equal comparing original.CreationTs (or the actual timestamp field on
original) to extracted.CreationTs so the round-trip validates the creation
timestamp as well.
- Around line 53-63: The test currently only checks sm.Blob is non-empty but
doesn't verify the provided timestamp was used; update
TestBuildACPSuppliedMetadataWithTime to decode/unmarshal sm.Blob into the same
SuppliedMetadata/protobuf type produced by BuildACPSuppliedMetadataWithTime and
assert the decoded metadata's block height equals 50 (the value set by
NewTimestamp(ts, 50)); use the existing ts and sm.Blob symbols to locate the
values to decode and add an assertion that the timestamp-derived block height is
exactly 50.
In `@x/acp/types/params_test.go`:
- Around line 17-21: Remove the redundant uint64 cast in TestDefaultParams:
update the assertion that compares DefaultPolicyCommandMaxExpirationDelta to
p.PolicyCommandMaxExpirationDelta in the TestDefaultParams function so it passes
DefaultPolicyCommandMaxExpirationDelta directly (no explicit type conversion),
leaving the equality check between DefaultPolicyCommandMaxExpirationDelta and
p.PolicyCommandMaxExpirationDelta unchanged.
In `@x/acp/types/time_test.go`:
- Around line 95-97: The test currently uses require.Panics for ts.IsAfter which
accepts any panic; change it to assert the exact panic value by using
require.PanicsWithValue and the literal panic string "invalid duration" so the
test locks the contract of IsAfter (the panic raised in the IsAfter
implementation).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2b396f64-0b40-4a69-91cc-06ab4c4ddac1
📒 Files selected for processing (25)
sdk/listener.goutils/sort.goutils/span.gox/acp/capability/manager_test.gox/acp/capability/types_test.gox/acp/did/did_test.gox/acp/did/types_test.gox/acp/metrics/metrics_test.gox/acp/simulation/simulation_test.gox/acp/stores/kv_stores_cosmos_test.gox/acp/stores/marshaler_test.gox/acp/types/access_decision_test.gox/acp/types/codec_test.gox/acp/types/commitment_test.gox/acp/types/constants_test.gox/acp/types/errors_test.gox/acp/types/keys_test.gox/acp/types/mapper_test.gox/acp/types/messages_test.gox/acp/types/params_test.gox/acp/types/policy_cmd_test.gox/acp/types/time_test.gox/acp/utils/utils_test.gox/hub/keeper/jws_token.gox/tier/keeper/credit.go
📜 Review details
🧰 Additional context used
🪛 Betterleaks (1.1.1)
x/acp/did/types_test.go
[high] 17-17: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (44)
x/acp/metrics/metrics_test.go (2)
3-7: Clean test setup and imports.Imports are minimal and appropriate for this unit test.
9-17: Good regression guard for metric-name constants.This test is straightforward and valuable: it will immediately catch accidental renames of exported metric identifiers.
x/tier/keeper/credit.go (1)
215-215: Correct immutable accumulation fix in total credit tracking.Line 215 now correctly reassigns
totalCreditafterAdd, so the value persisted at Line 219 reflects the real epoch total instead of remaining zero. Nice bug fix.utils/sort.go (1)
72-73: Correct allocation length inSort()Good fix. Using
make([]T, len(s.ts))makescopy(vals, s.ts)actually copy elements, so sorting now operates on real data and returns correct results.x/acp/types/errors_test.go (4)
10-15: Good validation for invalid account address constructor behavior.This test cleanly verifies both error creation and user-facing message content.
17-21: Good coverage for account-not-found constructor.Clear and focused assertion on the expected error surface.
23-26: Nice check for exported sentinel error wiring.Verifying
ErrInvalidSigneris non-nil and message-bearing is useful regression coverage.
28-34: Enum distinction checks look good.These assertions help catch accidental value collisions in
ErrorType.x/acp/types/access_decision_test.go (4)
12-16: Deterministic timestamp helper looks solid.Using a fixed UTC timestamp here keeps downstream ID/hash tests stable and reproducible.
18-46: Good determinism coverage forProduceId().This test correctly validates both non-empty output and repeated-call stability on the same input object.
48-78: Good uniqueness check with controlled input change.Varying
PolicyIdwhile keeping the rest aligned is a clean way to verify input-sensitive ID generation.
80-91:hashParams()assertions are concise and meaningful.Checking both non-empty output and 32-byte length gives strong basic coverage for SHA-256-shaped output.
x/acp/types/messages_test.go (5)
1-8: LGTM!Package declaration and imports are appropriate. Using
testify/requirefor assertions is a good practice for test clarity and fail-fast behavior.
10-17: LGTM!The test correctly validates all fields set by
NewMsgBearerPolicyCmd. Note thatNewRegisterObjectCmdis used here but not explicitly tested with its own dedicated test function—consider adding one if full coverage of constructor functions is desired.
19-56: LGTM!These tests follow a consistent pattern and correctly verify that the constructor functions populate all expected fields. The
AccessRequestsetup inTestNewMsgCheckAccessis well-structured with appropriate test data.
58-70: LGTM!Both tests correctly verify the signed policy command constructors, including the convenience
NewMsgSignedPolicyCmdFromJWSfunction that implicitly sets theMsgSignedPolicyCmd_JWStype.
72-105: Well-structured table-driven test.Good use of the table-driven pattern with
t.Runfor subtests. The current cases cover the essential valid/invalid authority scenarios.
[approve_code_changes, suggest_optional_refactor]For more comprehensive validation coverage, you could optionally expand the test table with edge cases:
💡 Optional: Additional edge cases
tests := []struct { name string msg MsgUpdateParams wantErr bool }{ { "valid authority", MsgUpdateParams{ Authority: "cosmos10d07y265gmmuvt4z0w9aw880jnsr700j6zn9kn", Params: DefaultParams(), }, false, }, { "invalid authority", MsgUpdateParams{ Authority: "invalid", Params: DefaultParams(), }, true, }, + { + "empty authority", + MsgUpdateParams{ + Authority: "", + Params: DefaultParams(), + }, + true, + }, }x/acp/did/types_test.go (3)
11-31: Solid table-driven coverage for DID validation.This is clear and correctly exercises valid, invalid, and empty inputs.
33-41: Good happy-path coverage forIssueDID.The generated key/account setup and DID prefix assertion provide useful integration-level confidence.
49-57: Deterministic module/ICA DID format tests look good.These assertions are precise and low-maintenance.
x/acp/did/did_test.go (1)
17-23: Uniqueness test is valuable and correctly scoped.This is a good regression guard for accidental key reuse/stateful behavior.
x/acp/types/mapper_test.go (4)
19-32: Deterministic context helper looks solid.Nice setup for stable, reproducible metadata tests (fixed block header + in-memory store).
34-43:TestBuildRecordMetadatacovers core fields clearly.Good verification of owner/signer and block-height propagation.
45-51:TestBuildACPSuppliedMetadatais a good baseline sanity test.The nil/error/blob checks are appropriate for this path.
82-88: Invalid-blob negative case is good.This is a useful guardrail for protobuf decode failures.
x/acp/types/keys_test.go (1)
1-30: LGTM!These tests provide good regression protection for key constants and prefixes, preventing accidental changes that could break storage compatibility. The assertions are clear and correctly use
requirefor immediate failure.x/acp/types/constants_test.go (1)
14-18: LGTM!Good test to verify the duration wrapper type, ensuring the constant uses the expected protobuf duration representation.
x/acp/types/params_test.go (1)
23-30: The test correctly documents thatValidate()is a no-op for this module — the implementation inx/acp/types/params.goexplicitly returnsnilwithout performing any checks. This appears intentional and consistent with similar no-op implementations in other modules likex/hub/types/params.go.x/acp/utils/utils_test.go (3)
11-42: GreatHashTxcoverage and oracle-based assertions.These tests validate size, deterministic equality with
sha256.Sum256, and non-equality across different inputs—solid for regression protection.
44-81:WriteBytestests are thorough and behavior-focused.Nice coverage of nil/empty/non-empty inputs plus explicit length-prefix impact via hash-state comparison.
20-21: No changes needed. This repository targets Go 1.25.5, which includes the Go 1.22+ fix for range variable capture semantics in closures. Thetc := tcpattern is unnecessary.> Likely an incorrect or invalid review comment.x/acp/types/time_test.go (1)
11-88: Great coverage across bothDurationvariants and boundary semantics.These tests exercise constructor shape, ISO formatting, and before/at/after expiry behavior for both block-count and proto-duration paths.
Also applies to: 100-108
x/acp/types/commitment_test.go (1)
11-43: Nice table-driven coverage for proto-duration expiry behavior.The test clearly validates not-expired, boundary, and expired outcomes through
IsExpiredAgainstdelegation.x/acp/types/policy_cmd_test.go (2)
10-73: LGTM!The test functions are well-structured and follow a consistent pattern. They correctly:
- Type-assert the
cmd.Cmdfield to the expected protobuf oneof wrapper types- Verify payload fields match the constructor inputs
- Use
uint64casts forRegistrationsCommitmentIdandEventIdmatching the constructor signatures
75-82: No action needed.PolicyCmd_UnarchiveObjectCmdandNewUnarchiveObjectCmdare properly defined in the codebase and correctly used in this test.x/hub/keeper/jws_token.go (2)
37-56: Strong validation and typed-error migration.This segment is a solid cleanup: explicit nil/empty checks and consistent
types.ErrInvalidInputwrapping make caller behavior much clearer.
232-233: Consensus-time usage is correct here.Switching to
sdk.UnwrapSDKContext(ctx).BlockTime()for invalidation and usage timestamps improves determinism and avoids wall-clock drift in state transitions.Also applies to: 253-257, 310-310, 319-320
x/acp/capability/types_test.go (1)
1-22: LGTM!Clean unit tests for
PolicyCapabilitygetters. The test assertions correctly match the production implementation (e.g., the/acp/module_policies/prefix format confirmed intypes.go).x/acp/stores/kv_stores_cosmos_test.go (1)
1-100: LGTM!Good test coverage for the
cosmosKvWrapper. The tests validate key operations including thenilreturn for missing keys (matching the implementation inkv_stores_cosmos.go).Minor observation: The iterator test only checks count; verifying actual key/value content could strengthen coverage, but acceptable for this scope.
x/acp/simulation/simulation_test.go (1)
1-137: LGTM!Well-structured simulation tests with deterministic setup. The
FindAccounttests cover success, not-found, and panic cases appropriately. The simulation operation tests correctly verify the current stub behavior returns "not implemented".sdk/listener.go (4)
132-136: Correct fix:returnproperly exits the goroutine.Using
breakwithin aselectinside aforloop only breaks out of theselect, causing the loop to continue. Changing toreturnensures the goroutine terminates whenl.Done()orctx.Done()signals.
29-36: Typo fixes in comments look good.
114-118: Comment improvements are accurate.
156-178: Potential race condition depends on CometBFT'sStop()synchronization guarantees.The
sync.Onceprevents double-close but doesn't prevent send-after-close panics. However, the risk is mitigated by the cleanup order inClose(): callingl.rpc.Stop()beforel.cleanupFn()should close the subscription channel synchronously, causing thefor range chto exit and the goroutine'sdefer once.Do(doClose)to execute beforecleanupFn()closes the output channels.This safety depends on CometBFT's
Stop()closing subscriptions synchronously—a guarantee not explicitly documented in the codebase. To eliminate the theoretical risk entirely, consider adding aWaitGroupor explicit synchronization to ensure the goroutine completes beforecloseFn()closes the channels.
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/acp/capability/manager.go (1)
110-123:⚠️ Potential issue | 🟠 MajorHandle multiple non-ACP owners explicitly.
Lines 126-138 now make ACP membership sufficient, so a capability claimed by ACP + module A + module B will pass
Validate, butGetOwnerModulestill returns the first non-ACP owner at Line 123. Inx/acp/keeper/module_acp.goLines 67-72,EditModulePolicythen injects that module as the acting principal. Please reject thelen(mods) > 1case instead of silently picking one owner.🔧 Minimal guard for `GetOwnerModule`
func (m *PolicyCapabilityManager) GetOwnerModule(ctx sdk.Context, capability *PolicyCapability) (string, error) { mods, _, err := m.scopedKeeper.LookupModules(ctx, capability.GetCapabilityName()) if err != nil { return "", fmt.Errorf("looking up capability owner: %v", err) //TODO } mods = utils.FilterSlice(mods, func(name string) bool { return name != types.ModuleName }) if len(mods) == 0 { return "", errors.Wrap("capability not claimed by any module", ErrInvalidCapability) } + if len(mods) > 1 { + return "", errors.Wrap("capability claimed by multiple modules", ErrInvalidCapability) + } return mods[0], nil }Also applies to: 126-138
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x/acp/capability/manager.go` around lines 110 - 123, GetOwnerModule in PolicyCapabilityManager currently filters out ACP and returns the first remaining owner, which silently allows multiple non-ACP owners; change the logic in GetOwnerModule (PolicyCapabilityManager.GetOwnerModule) to detect if after filtering there are more than one module (len(mods) > 1) and return an error (wrap ErrInvalidCapability with a message like "capability claimed by multiple non-ACP modules") instead of returning mods[0], so callers (e.g., EditModulePolicy in module_acp.go) cannot inject an ambiguous acting principal.
♻️ Duplicate comments (1)
x/acp/capability/manager_test.go (1)
19-31:⚠️ Potential issue | 🟠 MajorUse a memory store for the capability mem key.
Line 21 creates
capkeepermemwithstoretypes.NewKVStoreKey, and Line 26 mounts it asstoretypes.StoreTypeDB.capabilitykeeper.NewKeeperuses that second key as a memory store, so this bootstrap does not match the keeper contract and can hide capability-store bugs.🔧 Suggested fix
- capMemStoreKey := storetypes.NewKVStoreKey("capkeepermem") + capMemStoreKey := storetypes.NewMemoryStoreKey("capkeepermem") ... - stateStore.MountStoreWithDB(capMemStoreKey, storetypes.StoreTypeDB, db) + stateStore.MountStoreWithDB(capMemStoreKey, storetypes.StoreTypeMemory, nil)For `github.com/cosmos/ibc-go/modules/capability/keeper.NewKeeper` on Cosmos SDK 0.50.x, should the keeper's mem store key be created with `storetypes.NewMemoryStoreKey` and mounted with `storetypes.StoreTypeMemory`? Please cite official documentation or package references.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x/acp/capability/manager_test.go` around lines 19 - 31, The test mounts the capability mem key as a DB store which violates the keeper contract; change capMemStoreKey to be created via storetypes.NewMemoryStoreKey and mount it as a memory store (use stateStore.MountStore with storetypes.StoreTypeMemory) so setupCapKeeper uses a true memory store for capMemStoreKey when calling capabilitykeeper.NewKeeper.
🧹 Nitpick comments (2)
x/acp/did/types_test.go (1)
38-40: Prefer prefix assertion for DID format contract.
require.Contains(t, did, "did:key:")can pass even when the prefix is misplaced; checking prefix makes this test stricter.✅ Tighten assertion
import ( + "strings" "testing" @@ did, err := IssueDID(acc) require.NoError(t, err) - require.Contains(t, did, "did:key:") + require.True(t, strings.HasPrefix(did, "did:key:"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x/acp/did/types_test.go` around lines 38 - 40, The test currently uses require.Contains(t, did, "did:key:") which can pass for misplaced substrings; change the assertion to verify the DID starts with the prefix by using strings.HasPrefix on the variable did (from the IssueDID call) and assert that is true (e.g., require.True(t, strings.HasPrefix(did, "did:key:"))), adding an import for "strings" if not present so the test strictly enforces the prefix contract.x/hub/keeper/msg_invalidate_jws_test.go (1)
96-97: Add a malformed-record case for the new decode error path.These assertions cover the happy path of the new
(record, found, err)contract, butx/hub/keeper/msg_invalidate_jws.gonow also has a decode-failure branch. A test that seeds invalid bytes for a token hash would keep the wrapped"decoding JWS token"behavior from regressing.Also applies to: 116-117
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x/hub/keeper/msg_invalidate_jws_test.go` around lines 96 - 97, Add a test case that seeds a malformed record for the token hash and asserts the decode-failure path: write invalid bytes under the JWS token store key for tokenHash1 (use the same store key construction as the keeper, e.g. the JWSToken key prefix + token hash) before calling GetJWSToken / the InvalidateJWS handler, then assert that the call returns an error and that the error message indicates a decoding failure (matches the "decoding JWS token" wrapped error path). This ensures the keeper method GetJWSToken and the InvalidateJWS flow handle and report decode errors for malformed stored bytes.
🤖 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/acp/did/types.go`:
- Around line 45-47: The current nil check only catches a nil interface but not
typed-nil concrete keys, so before dereferencing fields like sdkPubKey.Key.X in
the P256 branch (and similarly for other key type cases) add an explicit nil
check on the concrete asserted value: after the type assertion (e.g., sdkPubKey
:= pk.(*secp256r1.PubKey) or whatever concrete type is used) test if sdkPubKey
== nil and return a descriptive error if so; do this for every type assertion of
cryptotypes.PubKey in this function or switch so you never dereference a
typed-nil pointer.
In `@x/hub/keeper/jws_token.go`:
- Around line 323-324: The expiration boundary is inconsistent between token
creation and cleanup: update the conditional inside CheckAndUpdateExpiredTokens
that currently uses record.ExpiresAt.Before(currentTime) so it instead uses
!record.ExpiresAt.After(currentTime) (i.e. treat ExpiresAt <= currentTime as
expired) to match the creation check that uses
!expiresAt.After(sdk.UnwrapSDKContext(ctx).BlockTime()); modify the check on
record.ExpiresAt/currentTime in the CheckAndUpdateExpiredTokens function
accordingly so both paths treat expiresAt == currentTime the same.
---
Outside diff comments:
In `@x/acp/capability/manager.go`:
- Around line 110-123: GetOwnerModule in PolicyCapabilityManager currently
filters out ACP and returns the first remaining owner, which silently allows
multiple non-ACP owners; change the logic in GetOwnerModule
(PolicyCapabilityManager.GetOwnerModule) to detect if after filtering there are
more than one module (len(mods) > 1) and return an error (wrap
ErrInvalidCapability with a message like "capability claimed by multiple non-ACP
modules") instead of returning mods[0], so callers (e.g., EditModulePolicy in
module_acp.go) cannot inject an ambiguous acting principal.
---
Duplicate comments:
In `@x/acp/capability/manager_test.go`:
- Around line 19-31: The test mounts the capability mem key as a DB store which
violates the keeper contract; change capMemStoreKey to be created via
storetypes.NewMemoryStoreKey and mount it as a memory store (use
stateStore.MountStore with storetypes.StoreTypeMemory) so setupCapKeeper uses a
true memory store for capMemStoreKey when calling capabilitykeeper.NewKeeper.
---
Nitpick comments:
In `@x/acp/did/types_test.go`:
- Around line 38-40: The test currently uses require.Contains(t, did,
"did:key:") which can pass for misplaced substrings; change the assertion to
verify the DID starts with the prefix by using strings.HasPrefix on the variable
did (from the IssueDID call) and assert that is true (e.g., require.True(t,
strings.HasPrefix(did, "did:key:"))), adding an import for "strings" if not
present so the test strictly enforces the prefix contract.
In `@x/hub/keeper/msg_invalidate_jws_test.go`:
- Around line 96-97: Add a test case that seeds a malformed record for the token
hash and asserts the decode-failure path: write invalid bytes under the JWS
token store key for tokenHash1 (use the same store key construction as the
keeper, e.g. the JWSToken key prefix + token hash) before calling GetJWSToken /
the InvalidateJWS handler, then assert that the call returns an error and that
the error message indicates a decoding failure (matches the "decoding JWS token"
wrapped error path). This ensures the keeper method GetJWSToken and the
InvalidateJWS flow handle and report decode errors for malformed stored bytes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d4fc8af2-c42e-4e9f-8873-47c1bdd84c63
📒 Files selected for processing (8)
utils/span.gox/acp/capability/manager.gox/acp/capability/manager_test.gox/acp/did/types.gox/acp/did/types_test.gox/hub/keeper/jws_token.gox/hub/keeper/msg_invalidate_jws.gox/hub/keeper/msg_invalidate_jws_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- utils/span.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🧰 Additional context used
🪛 Betterleaks (1.1.1)
x/acp/did/types_test.go
[high] 17-17: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (7)
x/acp/did/types_test.go (1)
43-47: Nice regression coverage for nil pubkey behavior.This test now validates the explicit error contract instead of panic behavior.
x/acp/capability/manager_test.go (2)
42-89: Nice smoke coverage for the manager basics.The constructor plus
Issue,Fetch, andClaimhappy/error paths are all exercised here, which should catch straightforward regressions quickly.
91-145: Good regression coverage around the ownership fix.These cases pin down ACP-only validation, ACP+claimer validation, and the no-claimer
GetOwnerModulefailure path.x/hub/keeper/msg_invalidate_jws.go (1)
21-24: Nice handling of the newGetJWSTokenerror return.This keeps decode/corruption failures distinct from the normal not-found path, which matches the keeper contract change cleanly.
x/hub/keeper/jws_token.go (3)
8-8: ConsistentErrInvalidInputwrapping looks good.Standardizing these validation failures onto one sentinel should make caller handling and assertions less ad hoc.
Also applies to: 38-55
88-100: Good fix: corrupt records no longer look like missing tokens.The new three-value
GetJWSTokencontract and updated call sites preserve real misses while surfacing decode failures immediately.Also applies to: 106-109, 173-176, 203-206, 233-239, 260-266, 313-316
244-245: Using consensus time for persisted timestamps is the right change.
InvalidatedAt,FirstUsedAt, andLastUsedAtare now deterministic across validators instead of depending on local wall clock.Also applies to: 268-272, 328-338
| if pk == nil { | ||
| return "", fmt.Errorf("account public key is nil") | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Go, if an interface value holds a typed nil pointer (e.g., var p *T=nil; var i I=p), does i == nil evaluate true or false?
💡 Result:
In Go, if an interface value holds a typed nil pointer (e.g., var p *T = nil; var i I = p), then i == nil evaluates to false.
Citations:
- 1: https://go.dev/doc/faq
- 2: https://hnphan.github.io/Typed-Nil-In-Go/
- 3: https://medium.com/@ben.meehan_27368/understanding-nil-in-go-interfaces-typed-nil-and-common-pitfalls-6b1154718e00
- 4: https://blog.kowalczyk.info/a-d9qh/explaining-nil-interface-gotcha-in-go.html
- 5: https://stackoverflow.com/questions/60733102/golangconfused-at-nil-pointer-interface-and-nil-interface
🏁 Script executed:
cd x/acp/did && cat -n types.go | head -100Repository: sourcenetwork/sourcehub
Length of output: 3328
Guard against typed-nil cryptotypes.PubKey values in type assertions, not only nil interfaces.
The if pk == nil check at line 45 does not catch interface values holding typed-nil pointers (e.g., (*secp256r1.PubKey)(nil)). When the P256 case matches, line 67 dereferences sdkPubKey.Key.X on a potentially nil pointer, causing a panic. Similar risks exist for all key types.
🔧 Suggested fix
func DIDFromPubKey(pk cryptotypes.PubKey) (string, error) {
if pk == nil {
return "", fmt.Errorf("account public key is nil")
}
var keyType crypto.KeyType
switch t := pk.(type) {
case *secp256k1.PubKey:
+ if t == nil {
+ return "", fmt.Errorf("account public key is nil")
+ }
keyType = crypto.SECP256k1
case *secp256r1.PubKey:
+ if t == nil {
+ return "", fmt.Errorf("account public key is nil")
+ }
keyType = crypto.P256
case *ed25519.PubKey:
+ if t == nil {
+ return "", fmt.Errorf("account public key is nil")
+ }
keyType = crypto.Ed25519
default:
return "", fmt.Errorf(
"failed to issue did for key %v: account key type must be secp256k1, secp256r1, or ed25519, got %v",
pk.Bytes(), t,
)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@x/acp/did/types.go` around lines 45 - 47, The current nil check only catches
a nil interface but not typed-nil concrete keys, so before dereferencing fields
like sdkPubKey.Key.X in the P256 branch (and similarly for other key type cases)
add an explicit nil check on the concrete asserted value: after the type
assertion (e.g., sdkPubKey := pk.(*secp256r1.PubKey) or whatever concrete type
is used) test if sdkPubKey == nil and return a descriptive error if so; do this
for every type assertion of cryptotypes.PubKey in this function or switch so you
never dereference a typed-nil pointer.
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/hub/keeper/jws_token.go`:
- Line 289: The sweeper currently treats zero-valued ExpiresAt as expired
because the condition uses !record.ExpiresAt.After(currentTime); update the
check to skip cleanup for non-expiring tokens by first checking
ExpiresAt.IsZero(), e.g., only consider expiration when ExpiresAt is non-zero
and not after currentTime. Modify the logic around record.ExpiresAt in the
sweep/delete path (the same token record handling that pairs with
StoreOrUpdateJWSToken) so that non-expiring tokens (ExpiresAt.IsZero()) are
preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4548645f-8b55-465b-b36b-1419b4e7eb05
📒 Files selected for processing (1)
x/hub/keeper/jws_token.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (2)
x/hub/keeper/jws_token.go (2)
86-100: Good fix: decode errors are no longer conflated with not-found.The
GetJWSToken3-return contract and updated callers correctly separate decode failure from missing-key behavior, and preserveErrJWSTokenNotFoundfor true misses.Also applies to: 106-112, 233-239, 260-266, 313-316
244-245: Good fix: consensus time is now used consistently.Switching to
sdk.UnwrapSDKContext(ctx).BlockTime()for invalidation/usage timestamps avoids non-deterministic wall-clock behavior.Also applies to: 268-272, 328-328, 337-338
|
|
||
| // Check if token is expired | ||
| if record.ExpiresAt.Before(currentTime) { | ||
| if !record.ExpiresAt.After(currentTime) { |
There was a problem hiding this comment.
Zero ExpiresAt is treated as expired during cleanup.
!record.ExpiresAt.After(currentTime) also matches zero time, so non-expiring tokens (ExpiresAt.IsZero()) will be invalidated by the sweeper. This conflicts with StoreOrUpdateJWSToken, which explicitly allows zero expiresAt as non-expiring (Line 323 guard).
💡 Proposed fix
- if !record.ExpiresAt.After(currentTime) {
+ if !record.ExpiresAt.IsZero() && !record.ExpiresAt.After(currentTime) {
// Mark as invalid
if err := k.UpdateJWSTokenStatus(ctx, record.TokenHash, types.JWSTokenStatus_STATUS_INVALID, ""); err != nil {
k.Logger().Error("failed to update expired token status", "hash", record.TokenHash, "error", err)
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@x/hub/keeper/jws_token.go` at line 289, The sweeper currently treats
zero-valued ExpiresAt as expired because the condition uses
!record.ExpiresAt.After(currentTime); update the check to skip cleanup for
non-expiring tokens by first checking ExpiresAt.IsZero(), e.g., only consider
expiration when ExpiresAt is non-zero and not after currentTime. Modify the
logic around record.ExpiresAt in the sweep/delete path (the same token record
handling that pairs with StoreOrUpdateJWSToken) so that non-expiring tokens
(ExpiresAt.IsZero()) are preserved.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev #98 +/- ##
==========================================
+ Coverage 47.84% 48.48% +0.64%
==========================================
Files 276 276
Lines 16195 16216 +21
==========================================
+ Hits 7749 7863 +114
+ Misses 7642 7543 -99
- Partials 804 810 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
Description
This PR fixes multiple bugs, cleans up related code, and adds tests.