Conversation
Co-authored-by: Anton Filonenko <philanton@users.noreply.github.com>
Contributor
📝 WalkthroughWalkthroughAdds an Action Gateway: DB schema for staking and action logs, new GatedAction domain types, store methods, gateway logic enforcing per-user/per-action 24h allowances based on staked tokens and app maintenance, config/schema files, API/CLI wiring, SDK changes, and extensive tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant AG as ActionGateway
participant Store as Store/DB
participant Calc as AllowanceCalc
User->>AG: AllowAction(wallet, gatedAction)
activate AG
AG->>Store: GetTotalUserStaked(wallet)
activate Store
Store-->>AG: stakedAmount
deactivate Store
AG->>Store: GetAppCount(wallet)
activate Store
Store-->>AG: appCount
deactivate Store
AG->>Calc: compute remainingStaked = staked - (appCount * appCost)
activate Calc
Calc-->>AG: remainingStaked
deactivate Calc
AG->>Calc: stakedTo24hActionsAllowance(gatedAction, remainingStaked)
activate Calc
Calc-->>AG: allowance
deactivate Calc
AG->>Store: GetUserActionCount(wallet, gatedAction, 24h)
activate Store
Store-->>AG: usedCount
deactivate Store
alt usedCount < allowance
AG->>Store: RecordAction(wallet, gatedAction)
activate Store
Store-->>AG: recordSuccess
deactivate Store
AG-->>User: allowed
else usedCount >= allowance
AG-->>User: denied (limit exceeded)
end
deactivate AG
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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 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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clearnode/config/migrations/postgres/20251222000000_initial_schema.sql`:
- Around line 287-288: The DOWN migration currently drops
idx_action_log_v1_wallet_method_created and
idx_action_log_v1_wallet_method_path_created but does not match the index name
introduced by the UP migration; update the DROP INDEX statements so they exactly
match the renamed index(s) created in the UP migration (replace
idx_action_log_v1_wallet_method_created with the corrected index name used in
the UP migration and ensure idx_action_log_v1_wallet_method_path_created matches
exactly), so rollback will remove the new index created by the schema fix.
- Around line 275-289: The DB schema for action_log_v1 uses columns method and
path but the Go model and queries expect gated_action (see
clearnode/store/database/action_log.go), causing undefined-column errors; update
the CREATE TABLE to replace method SMALLINT and path SMALLINT with a single
gated_action SMALLINT NOT NULL (or the exact type used in the Go model), then
update the indexes (e.g., idx_action_log_v1_wallet_method_path_created and
idx_action_log_v1_wallet_method_created) to reference gated_action (rename
indexes if desired) and adjust the Down section to DROP the updated index names
and table accordingly so the SQL matches the Go model.
In `@clearnode/store/database/action_log.go`:
- Around line 24-31: Reject invalid gated action IDs and non-positive windows in
DBStore.RecordAction: check gatedAction.ID() and the action window (e.g.,
gatedAction.Window()) before constructing ActionLogEntryV1 or performing any DB
queries/writes; if gatedAction.ID() == 0 or the window <= 0 return a descriptive
error immediately (do not proceed to create ActionLogEntryV1 or call DB methods)
so unknown actions and non-positive windows cannot silently distort counts.
In `@clearnode/store/database/interface.go`:
- Around line 211-213: Update the stale doc comment for GetUserActionCount to
match its signature: replace references to "method" and "path" with
"gatedAction" and clearly state that the function counts actions for the given
wallet and gatedAction within the provided time window (time.Duration), and
returns the number of matching actions and an error; ensure the comment sits
immediately above the GetUserActionCount declaration so it accurately documents
the wallet, gatedAction, and window parameters.
In `@clearnode/store/database/user_staked.go`:
- Around line 25-33: Validate inputs at the start of DBStore.UpdateUserStaked:
ensure the wallet (after strings.ToLower) is not empty, blockchainID is > 0, and
amount is not negative (use decimal's IsNegative or equivalent); if any check
fails return a descriptive error before constructing UserStakedV1 or performing
the upsert so invalid wallets, zero chain IDs, or negative amounts cannot be
written.
- Around line 13-15: The struct fields UserWallet and BlockchainID in
user_staked.go are missing gorm `primaryKey` tags, so AutoMigrate won't create
the composite primary key required by the PostgreSQL migration and the
OnConflict upsert (lines ~37-40) may target the wrong constraint; update the
struct tag for UserWallet (`gorm:"column:user_wallet;not null;primaryKey"`) and
for BlockchainID (`gorm:"column:blockchain_id;not null;primaryKey"`) so GORM
recognizes the composite primary key during AutoMigrate and keeps behavior
consistent with the UserBalance pattern.
In `@clearnode/store/memory/action_limits_store.go`:
- Around line 31-33: When config loading/decoding fails in the ActionLimitsStore
creation path, the code currently returns a non-nil empty &ActionLimitsStore{}
with an error; change those error returns to return nil, err so callers can't
mistakenly use a non-nil-but-invalid store. Update the return sites referenced
in action_limits_store.go (the error branch after the decode/load failures
around the current returns at lines returning &ActionLimitsStore{}) to return
nil, err, ensuring functions that construct or load ActionLimitsStore (e.g., the
constructor/loader that decodes config into ActionLimitsStore) propagate nil on
error.
- Around line 44-52: NewActionLimitsStoreFromConfig validates config but returns
an empty ActionLimitsStore, leaving the validated config unused; update the
constructor (NewActionLimitsStoreFromConfig) to populate the store's config
field (e.g., set the ActionLimitsStore.config or equivalent struct fields) with
the validated ActionLimitConfig so subsequent methods (that read
LevelStepTokens, AppCost, etc.) use the correct values and avoid zero-value
panics; ensure any other internal derived fields are initialized as needed
before returning the store.
- Around line 55-57: The StakedToAppCount method can return a huge uint if
stakedYellowTokens.Div(a.config.AppCost).IntPart() is negative; update
ActionLimitsStore.StakedToAppCount to mirror the defensive checks in
StakedTo24hActionsAllowance and StakedTo24hActions: compute the int64 value from
stakedYellowTokens.Div(a.config.AppCost).IntPart(), if it is negative return 0,
otherwise cast to uint and return it, referencing the same a.config.AppCost and
the StakedToAppCount method name so the change is localized and consistent.
In `@pkg/core/types.go`:
- Around line 912-915: The constants AppSessionCreation and AppSessionDeposit
currently share the same string value causing ID() to misclassify actions;
change AppSessionCreation to a distinct value (e.g., "app_session_creation") and
audit the neighboring GatedAction constants (the block around
AppSessionCreation/AppSessionOperation/AppSessionDeposit/AppSessionWithdrawal
and lines 919-930) to ensure each constant has a unique string literal; after
updating, confirm the ID() switch/lookup (the function that maps GatedAction
values to IDs) covers the corrected symbols so creation and deposit are
unambiguous.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
clearnode/config/migrations/postgres/20251222000000_initial_schema.sqlclearnode/store/database/action_log.goclearnode/store/database/database.goclearnode/store/database/interface.goclearnode/store/database/testing.goclearnode/store/database/user_staked.goclearnode/store/memory/action_limits_store.gopkg/core/types.go
clearnode/config/migrations/postgres/20251222000000_initial_schema.sql
Outdated
Show resolved
Hide resolved
Co-authored-by: Anton Filonenko <philanton@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (4)
pkg/core/types.go (1)
912-915:⚠️ Potential issue | 🔴 CriticalFix duplicated action value for creation vs deposit.
Line 912 and Line 914 both use
"app_session_deposit". This collapses two distinct actions into one value, soID()/GatedActionFromID()become inconsistent and misclassify creation/deposit actions.💡 Proposed fix
- GatedActionAppSessionCreation GatedAction = "app_session_deposit" + GatedActionAppSessionCreation GatedAction = "app_session_creation"Also applies to: 919-947
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/core/types.go` around lines 912 - 915, The enum constant GatedActionAppSessionCreation currently shares the same string value as GatedActionAppSessionDeposit ("app_session_deposit"), which collapses two distinct actions and breaks ID()/GatedActionFromID() mapping; change the value of GatedActionAppSessionCreation to a unique identifier (e.g., "app_session_creation" or "app_session_create") and scan the nearby constants (including the block referenced around 919-947) for any other duplicated string values, updating them to unique IDs so that ID() and GatedActionFromID() produce consistent, one-to-one mappings.clearnode/store/database/interface.go (1)
214-216:⚠️ Potential issue | 🟡 MinorUpdate
GetUserActionCountdocs to match the signature.The comment still references
method/path; it should describewallet,gatedAction, andwindow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clearnode/store/database/interface.go` around lines 214 - 216, Update the comment for GetUserActionCount to accurately describe its parameters and behavior: replace the outdated "method/path" wording with descriptions for the wallet (string), gatedAction (core.GatedAction) and window (time.Duration) parameters and state that it returns the count of actions for the given wallet and gatedAction within the specified time window, along with an error if applicable; update the comment immediately above the GetUserActionCount declaration to reflect this.clearnode/store/database/action_log.go (2)
24-31:⚠️ Potential issue | 🟠 MajorReject unknown gated actions before insert.
Line 30 persists
gatedAction.ID()without validation. IfID()==0, invalid actions get stored and later dropped bycore.GatedActionFromID, skewing usage data.💡 Suggested fix
func (s *DBStore) RecordAction(wallet string, gatedAction core.GatedAction) error { wallet = strings.ToLower(wallet) + actionID := gatedAction.ID() + if actionID == 0 { + return fmt.Errorf("unknown gated action: %s", gatedAction) + } entry := ActionLogEntryV1{ ID: uuid.New(), UserWallet: wallet, - GatedAction: gatedAction.ID(), + GatedAction: actionID, CreatedAt: time.Now(), }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clearnode/store/database/action_log.go` around lines 24 - 31, RecordAction is persisting gatedAction.ID() without validation, allowing ID()==0 (unknown) entries to be stored; validate the gatedAction before inserting by checking its ID (e.g., gatedAction.ID()==0) or resolving it with core.GatedActionFromID and returning an error if it yields an unknown/invalid action, and only construct and insert ActionLogEntryV1 when the gated action is valid.
43-49:⚠️ Potential issue | 🟠 MajorValidate
window(and action ID) before running count queries.Line 45/60 accepts non-positive windows, which can silently undercount actions. Also, Line 48 should reject
gatedAction.ID()==0before querying.💡 Suggested fix
func (s *DBStore) GetUserActionCount(wallet string, gatedAction core.GatedAction, window time.Duration) (uint64, error) { + if window <= 0 { + return 0, fmt.Errorf("window must be positive") + } wallet = strings.ToLower(wallet) + actionID := gatedAction.ID() + if actionID == 0 { + return 0, fmt.Errorf("unknown gated action: %s", gatedAction) + } since := time.Now().Add(-window) query := s.db.Model(&ActionLogEntryV1{}). - Where("user_wallet = ? AND gated_action = ? AND created_at >= ?", wallet, gatedAction.ID(), since) + Where("user_wallet = ? AND gated_action = ? AND created_at >= ?", wallet, actionID, since) @@ func (s *DBStore) GetUserActionCounts(userWallet string, window time.Duration) (map[core.GatedAction]uint64, error) { + if window <= 0 { + return nil, fmt.Errorf("window must be positive") + } userWallet = strings.ToLower(userWallet) since := time.Now().Add(-window)Also applies to: 58-65
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clearnode/store/database/action_log.go` around lines 43 - 49, GetUserActionCount currently allows non-positive window durations and zero action IDs which can produce incorrect counts; before building the query check that window > 0 and gatedAction.ID() != 0 and return a descriptive error if either is invalid. Update GetUserActionCount (and the sibling function around the following block that performs a similar count) to validate the window and gatedAction.ID() early, avoid running the DB query when invalid, and return a clear error explaining which parameter was bad so callers can handle it.
🧹 Nitpick comments (1)
clearnode/store/database/action_log_test.go (1)
64-80: Add regression coverage for creation vs deposit action identity.Current filtering tests don’t validate that
app_session_creationandapp_session_depositare counted independently. Add both actions in the same test to prevent ID/value-collision regressions.💡 Example enhancement
require.NoError(t, store.RecordAction("0xuser", core.GatedActionTransfer)) require.NoError(t, store.RecordAction("0xuser", core.GatedActionTransfer)) require.NoError(t, store.RecordAction("0xuser", core.GatedActionAppSessionOperation)) + require.NoError(t, store.RecordAction("0xuser", core.GatedActionAppSessionCreation)) + require.NoError(t, store.RecordAction("0xuser", core.GatedActionAppSessionDeposit)) @@ opCount, err := store.GetUserActionCount("0xuser", core.GatedActionAppSessionOperation, time.Hour) require.NoError(t, err) assert.Equal(t, uint64(1), opCount) + + creationCount, err := store.GetUserActionCount("0xuser", core.GatedActionAppSessionCreation, time.Hour) + require.NoError(t, err) + assert.Equal(t, uint64(1), creationCount) + + depositCount, err := store.GetUserActionCount("0xuser", core.GatedActionAppSessionDeposit, time.Hour) + require.NoError(t, err) + assert.Equal(t, uint64(1), depositCount) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clearnode/store/database/action_log_test.go` around lines 64 - 80, Add regression coverage by updating the "filters by gated action" subtest to record both app session creation and app session deposit actions and assert they are counted independently; specifically, in the test that calls store.RecordAction and store.GetUserActionCount, add store.RecordAction("0xuser", core.GatedActionAppSessionCreation) and store.RecordAction("0xuser", core.GatedActionAppSessionDeposit) (or the exact enum names used in core) and then call GetUserActionCount for each (core.GatedActionAppSessionCreation and core.GatedActionAppSessionDeposit) asserting expected counts so creation and deposit do not collide with app_session_operation or transfer counts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clearnode/action_gateway/action_gateway_test.go`:
- Around line 167-175: The test currently expects unknown actions to be allowed;
update it to assert that gw.AllowAction returns an error for an unconfigured
action and that mockAVStore (mockAVStore.recordedActions) is not called; locate
the test using mustNewGateway and AllowAction and change require.NoError to
require.Error (or require.EqualError with the expected message) and keep the
assertion that store.recordedActions is empty to ensure no store interaction
occurs.
In `@clearnode/action_gateway/action_gateway.go`:
- Around line 75-77: Update the comment for GetUserActionCount to reflect its
current parameters and behavior: replace the outdated "method/path" wording with
that it accepts a gatedAction (core.GatedAction) and a time window
(time.Duration) and returns the number of actions for the given wallet matching
the gatedAction within the specified window; keep the brief description and
signature alignment with GetUserActionCount(wallet string, gatedAction
core.GatedAction, window time.Duration) (uint64, error).
- Around line 106-117: The current flow uses tx.GetUserActionCount(...) then
tx.RecordAction(...), which is a TOCTOU race; change this to an atomic
check-and-record operation inside the same DB transaction by adding a new method
on tx (e.g., RecordActionIfBelowLimit or IncrementActionCountIfAllowed) that
accepts userAddress, gatedAction, defaultTimeWindow and allowance and returns an
error if the limit would be exceeded; replace the separate GetUserActionCount +
RecordAction calls with a single call to that new method so the check and
increment happen atomically inside the database layer.
In `@clearnode/config/schemas/action_gateway_schema.yaml`:
- Around line 10-14: The schema patterns for the decimal fields (the pattern
under the "description: Decimal amount for level step tokens" and the "app_cost"
pattern) currently allow negative values; update both regexes to forbid a
leading '-' and to reject zero so only positive decimals > 0 pass validation
(e.g. replace the existing "^-?[0-9]+(\\.[0-9]+)?$" with a regex that disallows
negatives and all-zero values such as "^(?!0+(?:\\.0+)?$)[0-9]+(\\.[0-9]+)?$").
Ensure both the pattern for the level-step amount and the app_cost pattern are
changed consistently.
In `@clearnode/store/database/interface.go`:
- Around line 206-207: Update the comment above the method to match the actual
method name GetTotalUserStaked: replace the incorrect reference to GetUserStaked
with GetTotalUserStaked and ensure the description still accurately states it
returns the total staked amount for a user across all blockchains; modify the
comment that precedes the GetTotalUserStaked(wallet string) (decimal.Decimal,
error) declaration accordingly.
---
Duplicate comments:
In `@clearnode/store/database/action_log.go`:
- Around line 24-31: RecordAction is persisting gatedAction.ID() without
validation, allowing ID()==0 (unknown) entries to be stored; validate the
gatedAction before inserting by checking its ID (e.g., gatedAction.ID()==0) or
resolving it with core.GatedActionFromID and returning an error if it yields an
unknown/invalid action, and only construct and insert ActionLogEntryV1 when the
gated action is valid.
- Around line 43-49: GetUserActionCount currently allows non-positive window
durations and zero action IDs which can produce incorrect counts; before
building the query check that window > 0 and gatedAction.ID() != 0 and return a
descriptive error if either is invalid. Update GetUserActionCount (and the
sibling function around the following block that performs a similar count) to
validate the window and gatedAction.ID() early, avoid running the DB query when
invalid, and return a clear error explaining which parameter was bad so callers
can handle it.
In `@clearnode/store/database/interface.go`:
- Around line 214-216: Update the comment for GetUserActionCount to accurately
describe its parameters and behavior: replace the outdated "method/path" wording
with descriptions for the wallet (string), gatedAction (core.GatedAction) and
window (time.Duration) parameters and state that it returns the count of actions
for the given wallet and gatedAction within the specified time window, along
with an error if applicable; update the comment immediately above the
GetUserActionCount declaration to reflect this.
In `@pkg/core/types.go`:
- Around line 912-915: The enum constant GatedActionAppSessionCreation currently
shares the same string value as GatedActionAppSessionDeposit
("app_session_deposit"), which collapses two distinct actions and breaks
ID()/GatedActionFromID() mapping; change the value of
GatedActionAppSessionCreation to a unique identifier (e.g.,
"app_session_creation" or "app_session_create") and scan the nearby constants
(including the block referenced around 919-947) for any other duplicated string
values, updating them to unique IDs so that ID() and GatedActionFromID() produce
consistent, one-to-one mappings.
---
Nitpick comments:
In `@clearnode/store/database/action_log_test.go`:
- Around line 64-80: Add regression coverage by updating the "filters by gated
action" subtest to record both app session creation and app session deposit
actions and assert they are counted independently; specifically, in the test
that calls store.RecordAction and store.GetUserActionCount, add
store.RecordAction("0xuser", core.GatedActionAppSessionCreation) and
store.RecordAction("0xuser", core.GatedActionAppSessionDeposit) (or the exact
enum names used in core) and then call GetUserActionCount for each
(core.GatedActionAppSessionCreation and core.GatedActionAppSessionDeposit)
asserting expected counts so creation and deposit do not collide with
app_session_operation or transfer counts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ae4735f3-9a5d-4ba7-8868-a8a729f2f4a1
📒 Files selected for processing (13)
clearnode/action_gateway/action_gateway.goclearnode/action_gateway/action_gateway_test.goclearnode/chart/config/prod/action_gateway.yamlclearnode/chart/config/sandbox/action_gateway.yamlclearnode/chart/config/uat/action_gateway.yamlclearnode/chart/config/v1-rc/action_gateway.yamlclearnode/config/migrations/postgres/20251222000000_initial_schema.sqlclearnode/config/schemas/action_gateway_schema.yamlclearnode/store/database/action_log.goclearnode/store/database/action_log_test.goclearnode/store/database/app.goclearnode/store/database/interface.gopkg/core/types.go
✅ Files skipped from review due to trivial changes (2)
- clearnode/chart/config/prod/action_gateway.yaml
- clearnode/chart/config/uat/action_gateway.yaml
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
clearnode/api/rpc_router.go (1)
107-143:⚠️ Potential issue | 🔴 CriticalRegister
user.v1.get_action_allowancesin the router.
userV1Handleris wired withactionGateway, but the new method is not bound inuserV1Group, so clients will get method-not-found.➕ Proposed routing fix
userV1Group := r.Node.NewGroup(rpc.UserV1Group.String()) userV1Group.Handle(rpc.UserV1GetBalancesMethod.String(), userV1Handler.GetBalances) userV1Group.Handle(rpc.UserV1GetTransactionsMethod.String(), userV1Handler.GetTransactions) +userV1Group.Handle(rpc.UserV1GetActionAllowancesMethod.String(), userV1Handler.GetActionAllowances)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clearnode/api/rpc_router.go` around lines 107 - 143, The router is missing registration for the new user.v1.get_action_allowances RPC; add a route binding in the same place as other user V1 routes by calling userV1Group.Handle(rpc.UserV1GetActionAllowancesMethod.String(), userV1Handler.GetActionAllowances) so the userV1Handler's GetActionAllowances method (and rpc.UserV1GetActionAllowancesMethod) are exposed to clients.
♻️ Duplicate comments (3)
pkg/core/types.go (1)
921-924:⚠️ Potential issue | 🔴 Critical
AppSessionCreationstill collides with deposit action literal.
GatedActionAppSessionCreationandGatedActionAppSessionDepositboth resolve to"app_session_deposit", so ID mapping is ambiguous and can misclassify actions.🛠️ Proposed fix
-var ( +const ( GatedActionTransfer GatedAction = "transfer" - GatedActionAppSessionCreation GatedAction = "app_session_deposit" + GatedActionAppSessionCreation GatedAction = "app_session_creation" GatedActionAppSessionOperation GatedAction = "app_session_operation" GatedActionAppSessionDeposit GatedAction = "app_session_deposit" GatedActionAppSessionWithdrawal GatedAction = "app_session_withdrawal" )Also applies to: 928-956
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/core/types.go` around lines 921 - 924, Two constants collide: GatedActionAppSessionCreation and GatedActionAppSessionDeposit both map to "app_session_deposit", causing ambiguous action IDs; update GatedActionAppSessionCreation to a unique literal (e.g., "app_session_creation") and scan the other GatedAction constants in the nearby block (lines covering the GatedActionAppSession* group, e.g., GatedActionAppSessionOperation, GatedActionAppSessionWithdrawal and any entries in the 928-956 range) to ensure each has a distinct string value so ID mapping is unambiguous.clearnode/action_gateway/action_gateway.go (2)
106-117:⚠️ Potential issue | 🔴 CriticalMake allowance check + action recording atomic.
GetUserActionCountfollowed byRecordActionintroduces a TOCTOU race under concurrent requests, allowing limit overruns.🔒 Suggested direction
- usedCount, err := tx.GetUserActionCount(userAddress, gatedAction, defaultTimeWindow) - if err != nil { - return fmt.Errorf("failed to get user action count: %w", err) - } - if usedCount >= allowance { - return fmt.Errorf("action %s limit reached: used %d of %d allowed in 24h", gatedAction, usedCount, allowance) - } - if err := tx.RecordAction(userAddress, gatedAction); err != nil { - return fmt.Errorf("failed to record action: %w", err) - } + // Use one DB operation that checks current usage and records the action atomically. + if err := tx.TryConsumeAction(userAddress, gatedAction, defaultTimeWindow, allowance); err != nil { + return fmt.Errorf("failed to consume action allowance: %w", err) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clearnode/action_gateway/action_gateway.go` around lines 106 - 117, The current pattern calling tx.GetUserActionCount and then tx.RecordAction causes a TOCTOU race under concurrency; replace with an atomic operation on the transaction layer (e.g., add and call a new tx.CheckAndRecordAction or tx.IncrementIfBelowLimit) that accepts userAddress, gatedAction, allowance and defaultTimeWindow and performs the count-check-and-insert/update within a single DB transaction or row-level lock; update the caller to use that new method and handle its error/limit return instead of separately using usedCount and RecordAction.
36-47:⚠️ Potential issue | 🟠 MajorValidate
ActionGateskeys at construction time.
NewActionGatewaycurrently validates numeric config but not whetheraction_gateskeys are recognized and non-colliding by ID, which can silently accept bad config.🛡️ Suggested hardening
func NewActionGateway(cfg ActionLimitConfig) (*ActionGateway, error) { if !cfg.LevelStepTokens.IsPositive() { return nil, errors.New("LevelStepTokens must be greater than zero") } if !cfg.AppCost.IsPositive() { return nil, errors.New("AppCost must be greater than zero") } + if len(cfg.ActionGates) == 0 { + return nil, errors.New("ActionGates must not be empty") + } + seen := make(map[uint8]core.GatedAction, len(cfg.ActionGates)) + for action := range cfg.ActionGates { + id := action.ID() + if id == 0 { + return nil, fmt.Errorf("unknown action_gates key: %q", action) + } + if prev, ok := seen[id]; ok && prev != action { + return nil, fmt.Errorf("duplicate gated action id %d for %q and %q", id, prev, action) + } + seen[id] = action + } return &ActionGateway{ cfg: cfg, }, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clearnode/action_gateway/action_gateway.go` around lines 36 - 47, NewActionGateway currently only checks numeric fields but must also validate ActionLimitConfig.ActionGates entries: in NewActionGateway iterate the cfg.ActionGates collection (referencing ActionLimitConfig and NewActionGateway on ActionGateway), verify each gate key is a recognized/allowed ID and accumulate a set to ensure no duplicate IDs, and return a descriptive error if any unknown keys or duplicate IDs are found; update the constructor to reject the config with an error listing offending keys so invalid or colliding action_gates cannot be silently accepted.
🧹 Nitpick comments (13)
cerebro/operator.go (1)
460-468: Validate unexpectedregister-appoption values.Right now any third argument other than
no-approvalis silently accepted and treated as approval-required. It’s better to reject unknown flags explicitly.Suggested hardening
case "register-app": if len(args) < 2 { fmt.Println("ERROR: Usage: register-app <app_id> [no-approval]") fmt.Println("INFO: Pass 'no-approval' as second arg to allow session creation without owner approval") return } - noApproval := len(args) >= 3 && args[2] == "no-approval" + if len(args) > 3 || (len(args) == 3 && args[2] != "no-approval") { + fmt.Println("ERROR: Usage: register-app <app_id> [no-approval]") + return + } + noApproval := len(args) == 3 o.registerApp(ctx, args[1], "", noApproval)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cerebro/operator.go` around lines 460 - 468, The register-app branch currently accepts any third arg silently; update the case "register-app" handling to explicitly validate the optional flag: only accept "no-approval" as a valid third argument and set noApproval accordingly (use args and set noApproval = true only when args[2] == "no-approval"); if a third arg exists and is not "no-approval", print an error message (reusing the existing usage text) and return instead of calling o.registerApp; keep the call to o.registerApp(ctx, args[1], "", noApproval).clearnode/api/user_v1/testing.go (1)
38-56: Make newMockStoremethods configurable viam.Called(...).Lines 38-56 currently hard-code zero/nil returns, which limits coverage for staking/allowance edge cases (non-zero stake, pre-used quota, error paths).
♻️ Example pattern for one method
func (m *MockStore) GetTotalUserStaked(wallet string) (decimal.Decimal, error) { - return decimal.Zero, nil + args := m.Called(wallet) + if args.Get(0) == nil { + return decimal.Zero, args.Error(1) + } + return args.Get(0).(decimal.Decimal), args.Error(1) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clearnode/api/user_v1/testing.go` around lines 38 - 56, The MockStore methods GetAppCount, GetTotalUserStaked, RecordAction, GetUserActionCount, and GetUserActionCounts currently return hard-coded zero/nil; update each to delegate to m.Called(...) and return the configured values and error from the testify/mock call (e.g. ret := m.Called(<same args>); extract numeric/map/decimal results with type assertions and nil-checks, and return ret.Error(1) for the error), taking care to convert/Get(0) safely for decimal.Decimal (fall back to decimal.Zero if nil) and maps (return nil if Get(0) is nil) so tests can configure non-zero/stubbed error cases via m.On(...).Return(...).pkg/rpc/client_test.go (1)
365-372: Add assertions for nestedAppDefinitionV1fields.Lines 365-372 set nested definition data, but the test currently doesn’t assert it. Adding a couple assertions will protect this contract change.
✅ Suggested assertion additions
resp, err := client.AppSessionsV1GetAppSessions(testCtxV1, rpc.AppSessionsV1GetAppSessionsRequest{}) require.NoError(t, err) assert.Len(t, resp.AppSessions, 1) assert.Equal(t, testAppSession, resp.AppSessions[0].AppSessionID) +assert.Equal(t, "0xapp", resp.AppSessions[0].AppDefinitionV1.Application) +assert.Equal(t, "1", resp.AppSessions[0].AppDefinitionV1.Nonce)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/rpc/client_test.go` around lines 365 - 372, The test sets nested AppDefinitionV1 but doesn't assert any of its fields; add assertions after the structure is created to verify AppDefinitionV1.Application == "0xapp", AppDefinitionV1.Nonce == "1", AppDefinitionV1.Quorum == 1, that len(AppDefinitionV1.Participants) == 1, and that the single participant has WalletAddress == testWalletV1 and SignatureWeight == 1 (check the fields on the rpc.AppDefinitionV1 and rpc.AppParticipantV1 instances you create).pkg/app/app_session_v1.go (1)
41-52: Consider documenting why some intents return empty GatedAction.The
CloseandRebalanceintents return an empty string, meaning they are not gated. This appears intentional but could benefit from a brief comment explaining the design decision.📝 Add clarifying comment
func (intent AppStateUpdateIntent) GatedAction() core.GatedAction { switch intent { case AppStateUpdateIntentOperate: return core.GatedActionAppSessionOperation case AppStateUpdateIntentDeposit: return core.GatedActionAppSessionDeposit case AppStateUpdateIntentWithdraw: return core.GatedActionAppSessionWithdrawal default: + // Close and Rebalance intents are not gated actions return "" } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/app/app_session_v1.go` around lines 41 - 52, Add a short explanatory comment to the AppStateUpdateIntent.GatedAction method clarifying that certain intents (e.g., Close and Rebalance) intentionally return an empty GatedAction (ungated) and why (they do not require gating or do not affect resources protected by gating), so future readers understand the design decision; update the comment near the switch or the default case in the GatedAction method of type AppStateUpdateIntent.clearnode/api/app_session_v1/testing.go (1)
146-164: Potential panic on nil return values in mock methods.The new mock methods
GetAppCount,GetTotalUserStaked, andGetUserActionCountperform type assertions without nil checks, unlikeGetUserActionCounts(lines 166-172) which correctly handles nil. If a test returnsnilfor the first argument, these will panic.♻️ Suggested defensive nil checks
func (m *MockStore) GetAppCount(ownerWallet string) (uint64, error) { args := m.Called(ownerWallet) + if args.Get(0) == nil { + return 0, args.Error(1) + } return args.Get(0).(uint64), args.Error(1) } func (m *MockStore) GetTotalUserStaked(wallet string) (decimal.Decimal, error) { args := m.Called(wallet) + if args.Get(0) == nil { + return decimal.Decimal{}, args.Error(1) + } return args.Get(0).(decimal.Decimal), args.Error(1) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clearnode/api/app_session_v1/testing.go` around lines 146 - 164, The mock methods GetAppCount, GetTotalUserStaked, and GetUserActionCount currently do unchecked type assertions on args.Get(0) which can panic if the mocked call returns nil; update each method (MockStore.GetAppCount, MockStore.GetTotalUserStaked, MockStore.GetUserActionCount) to first check if args.Get(0) == nil and if so return a safe zero value (uint64(0) for counts, decimal.Zero for the decimal) along with args.Error(1), otherwise perform the type assertion and return the asserted value plus args.Error(1).clearnode/api/app_session_v1/handler.go (1)
35-59: Consider validating required dependencies.The
NewHandlerfunction doesn't validate thatactionGatewayis non-nil. If a caller accidentally passesnil, this could cause a panic when gating is invoked later. Other dependencies likesignerface similar considerations.This is a minor consideration since the constructor is likely only called from controlled initialization code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clearnode/api/app_session_v1/handler.go` around lines 35 - 59, NewHandler should validate required dependencies instead of accepting nils; change its signature to return (*Handler, error), check that actionGateway, signer, useStoreInTx, assetStore, stateAdvancer, and statePacker (and any other mandatory deps) are non-nil, and return a clear error if any are nil; construct and return the Handler only when validations pass, and update callers to handle the error.sdk/ts/src/app/types.ts (1)
141-141: Consider a short deprecation bridge for this SDK shape change.Replacing flat fields with
appDefinitionis a breaking consumer-facing change. A temporary deprecated alias strategy can reduce migration friction for existing integrators.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/ts/src/app/types.ts` at line 141, The SDK changed from flat fields to a single appDefinition: AppDefinitionV1 which is breaking; add a temporary deprecation bridge by keeping the original flat field names as aliases that forward to appDefinition (either as read-only getters or computed properties) and mark them with `@deprecated` JSDoc comments so existing integrators continue to work while encouraging migration; update the type definition that contains appDefinition (and associated interfaces) to include these deprecated aliases mapping to the corresponding appDefinition.<property> values and ensure serialization/instantiation code (constructors/factory functions) populate both appDefinition and the deprecated aliases.clearnode/api/channel_v1/handler.go (1)
16-16: Add nil-guard in constructor to prevent panic on nilactionGateway.The
actionGatewayfield (line 16) is assigned in the constructor without validation. Line 33 ofsubmit_state.godereferencesh.actionGatewaywithout checking for nil, which will panic if nil is passed toNewHandler. While the single callsite inrpc_router.go:102passes the parameter directly without nil-check, adding a panic with a descriptive message in the constructor would catch the issue early and provide better debugging context.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clearnode/api/channel_v1/handler.go` at line 16, The constructor NewHandler should validate its actionGateway parameter and panic with a clear message if it's nil to fail fast; add a nil-guard in NewHandler that checks the incoming actionGateway (the field actionGateway on the handler type) and panic("nil actionGateway passed to NewHandler") or similar, so calls like the one in rpc_router.go that currently pass through won't cause a later nil dereference in submit_state.go where h.actionGateway is used.clearnode/api/apps_v1/testing.go (1)
48-50: Prefer returning an empty map in defaultGetUserActionCounts.Returning
nil, nilcan force extra nil handling in tests; an empty map is usually safer for default mock behavior.♻️ Proposed tweak
func (m *MockStore) GetUserActionCounts(_ string, _ time.Duration) (map[core.GatedAction]uint64, error) { - return nil, nil + return map[core.GatedAction]uint64{}, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clearnode/api/apps_v1/testing.go` around lines 48 - 50, The MockStore.GetUserActionCounts stub currently returns (nil, nil); change it to return an empty map and nil error to avoid forcing nil checks in tests — specifically update the GetUserActionCounts method on MockStore to return map[core.GatedAction]uint64{} instead of nil. Keep the signature and error return the same, only replace the nil map with an initialized empty map.clearnode/api/apps_v1/submit_app_version.go (1)
52-63: Normalize owner wallet once and reuse it for gating + persistence.Line 52 uses raw
req.App.OwnerWallet, while Line 59 lowercases it for storage. Reusing one normalized value avoids case-variant behavior in gating/accounting.♻️ Proposed refactor
- err := h.actionGateway.AllowAppRegistration(tx, req.App.OwnerWallet) + ownerWallet := strings.ToLower(req.App.OwnerWallet) + err := h.actionGateway.AllowAppRegistration(tx, ownerWallet) if err != nil { return rpc.NewError(err) } appEntry := app.AppV1{ ID: strings.ToLower(req.App.ID), - OwnerWallet: strings.ToLower(req.App.OwnerWallet), + OwnerWallet: ownerWallet, Metadata: req.App.Metadata, Version: version, CreationApprovalNotRequired: req.App.CreationApprovalNotRequired, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clearnode/api/apps_v1/submit_app_version.go` around lines 52 - 63, The code lowercases req.App.OwnerWallet when constructing app.AppV1 but calls h.actionGateway.AllowAppRegistration(tx, req.App.OwnerWallet) using the raw value, which can cause case-variant behavior; normalize the owner wallet once (e.g., ownerWallet := strings.ToLower(req.App.OwnerWallet)) and then pass that normalized ownerWallet into AllowAppRegistration and set appEntry.OwnerWallet to the same variable so both gating (AllowAppRegistration) and persistence (appEntry) use an identical, lowercased value.pkg/rpc/types.go (2)
150-152: Rename Go field toApplicationIDfor API consistency.The JSON tag is
application_id, but the Go field is stillApplication. Renaming keeps the public Go model aligned with the rest of theApplicationIDmigration.♻️ Proposed rename
type AppDefinitionV1 struct { - // Application is the application identifier from an app registry - Application string `json:"application_id"` + // ApplicationID is the application identifier from an app registry + ApplicationID string `json:"application_id"`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/rpc/types.go` around lines 150 - 152, The struct field Application is misnamed relative to its JSON tag and project convention; rename the Go field to ApplicationID in pkg/rpc/types.go (update the struct field formerly named Application to ApplicationID with the existing `json:"application_id"` tag) and then update all references/usages of Application across the package (functions, constructors, tests, serialization code) to use ApplicationID to keep API and Go model consistent.
190-191: PreferAppDefinitionas the field name inAppSessionInfoV1.
AppDefinitionV1 AppDefinitionV1is harder to consume than needed. A cleaner field name improves readability without changing wire format.🧹 Suggested cleanup
- // AppDefinitionV1 contains immutable application fields - AppDefinitionV1 AppDefinitionV1 `json:"app_definition"` + // AppDefinition contains immutable application fields + AppDefinition AppDefinitionV1 `json:"app_definition"`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/rpc/types.go` around lines 190 - 191, Rename the struct field in AppSessionInfoV1 from AppDefinitionV1 AppDefinitionV1 to a clearer identifier (e.g., AppDefinition AppDefinitionV1) while preserving the json tag `json:"app_definition"` and the type AppDefinitionV1 to avoid wire-format changes; update all references/usages of AppSessionInfoV1.AppDefinitionV1 to the new field name (AppDefinition) across the codebase and run `go build`/tests to catch missing references.clearnode/api/app_session_v1/rebalance_app_sessions_test.go (1)
153-164: TightenGetAppmocks to validate lookup correctness.These duplicated
GetApp(mock.Anything)expectations are too broad and don’t verify the application ID being queried. Prefer a single expectation with explicit args and cardinality (.Twice()), or separate.Once()expectations with concrete IDs.♻️ Proposed test hardening
- mockStore.On("GetApp", mock.Anything).Return(nil, nil) - ... - mockStore.On("GetApp", mock.Anything).Return(nil, nil) + mockStore.On("GetApp", "test-app").Return(nil, nil).Twice()Also applies to: 320-329, 677-686
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clearnode/api/app_session_v1/rebalance_app_sessions_test.go` around lines 153 - 164, The duplicated broad mock for GetApp should be tightened to assert the exact app IDs being looked up: replace the two mockStore.On("GetApp", mock.Anything).Return(nil, nil) calls with either a single mockStore.On("GetApp", expectedAppID).Return(nil, nil).Twice() or two separate mockStore.On("GetApp", appID1).Return(nil, nil).Once() and mockStore.On("GetApp", appID2).Return(nil, nil).Once(), using the concrete IDs used in the test (e.g., sessionID1's app ID and sessionID2's app ID) so the test validates the lookup arguments; adjust the other occurrences noted (lines around 320-329 and 677-686) similarly and keep the existing UpdateAppSession matcher intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clearnode/api/app_session_v1/rebalance_app_sessions.go`:
- Around line 106-115: The code currently skips gating when
tx.GetApp(appSession.ApplicationID) returns nil; change the logic in the block
around tx.GetApp / registeredApp so that a nil registeredApp does not bypass
authorization: if tx.GetApp returns an error, keep the current error return; if
it returns nil, return a gating/lookup error (e.g., rpc.Errorf("application not
found for id %v", appSession.ApplicationID)) instead of proceeding; only call
h.actionGateway.AllowAction(tx, registeredApp.App.OwnerWallet,
update.AppStateUpdate.Intent.GatedAction()) when registeredApp is non-nil.
In `@clearnode/api/app_session_v1/submit_app_state.go`:
- Around line 68-78: SubmitAppState currently skips calling
h.actionGateway.AllowAction when tx.GetApp(appSession.ApplicationID) returns
nil, which can silently bypass gating; change the logic in SubmitAppState so
that after calling tx.GetApp (registeredApp) you explicitly fail-closed if
registeredApp == nil by returning an rpc.Errorf or appropriate RPC error (same
style used in CreateAppSession) instead of skipping AllowAction, then call
h.actionGateway.AllowAction(tx, registeredApp.App.OwnerWallet,
appStateUpd.Intent.GatedAction()) and propagate any error via rpc.NewError as
before.
In `@clearnode/api/apps_v1/submit_app_version.go`:
- Around line 84-86: Inside the transaction closure in submit_app_version.go,
you're calling h.store.CreateApp(appEntry) which bypasses the transactional
handle; replace that call with tx.CreateApp(appEntry) (using the tx variable
from the surrounding transaction closure) so the app creation participates in
the transaction and will be rolled back on error.
In `@clearnode/api/channel_v1/submit_state.go`:
- Around line 33-39: The AllowAction check is race-prone because it reads the
action count before acquiring the user/state lock; move the call to
h.actionGateway.AllowAction so it executes after
tx.LockUserState(incomingState.UserWallet, incomingState.Asset) (i.e., perform
the allowance check while holding the lock), or alternatively modify the
underlying GetUserActionCount query used by AllowAction to use FOR UPDATE to
obtain a row-level lock; update any call sites and ensure RecordAction still
runs under the same lock/transaction (via actionGateway.RecordAction or
equivalent) to prevent concurrent overshoot.
In `@clearnode/api/user_v1/testing.go`:
- Around line 58-67: Update the top comment for MockActionGateway to accurately
describe its behavior: it returns the pre-set Allowances slice or the Err value
when GetUserAllowances is called, rather than "always allows actions." Mention
that setting Err simulates an error and setting Allowances controls the returned
allowances; reference MockActionGateway, GetUserAllowances, Allowances and Err
in the comment so readers understand how to use the mock.
In `@clearnode/store/database/app_session.go`:
- Line 55: CreateAppSession stores ApplicationID without normalizing to
lowercase (unlike SessionID and WalletAddress), causing mismatches with
StoreAppSessionKeyState which lowercases application IDs; update the code in
CreateAppSession to call strings.ToLower on session.ApplicationID before
assigning to ApplicationID so AppSessionV1 rows use a lowercase application_id
and joins in GetAppSessionKeyOwner against app_session_key_applications_v1
succeed consistently.
In `@docs/api.yaml`:
- Around line 460-462: The description for the time_window field is ambiguous;
update the time_window documentation to state the exact wire format (e.g., Go
duration serialization like "24h0m0s") or indicate that the service
normalizes/accepts short form ("24h") and then serializes to full Go duration;
specifically edit the time_window field description to either (a) document the
emitted/expected format as Go duration string such as "24h0m0s" or (b) note that
inputs like "24h" are normalized and the API returns "24h0m0s", so callers know
what to send and what to expect.
- Around line 341-343: The docs schema is using the field name "app_definition"
but runtime payloads and the app_session_info contract use "application_id";
update the API spec so the returned property matches runtime naming: rename the
field referenced as "app_definition" to "application_id" (or, if the intent is
to embed the full app_definition, ensure the embedded schema includes an
"application_id" property and that app_session_info references that schema), and
update the description to mention it carries the application_id; check the
"app_definition" schema/type and any references in app_session_info to keep
names consistent.
In `@sdk/go/user.go`:
- Around line 109-114: Update transformActionAllowances to return
([]core.ActionAllowance, error) and stop ignoring strconv.ParseUint errors: in
transformActionAllowances iterate over allowances and for each a use
strconv.ParseUint on a.Allowance and a.Used, check both err values and if
non-nil return a clear error with context (e.g., include the offending field
name and an identifier from the rpc.ActionAllowanceV1 such as an Action or ID
field) instead of using “_”, and only append the core.ActionAllowance when both
parses succeed; propagate the error up so callers can handle malformed values.
In `@sdk/ts/src/utils.ts`:
- Line 359: The transformAppDefinitionFromRPC call is using a fallback {} which
lets transformAppDefinitionFromRPC (or its internal BigInt(raw.nonce)
conversion) run against missing fields and throw; update the code so you first
check that raw.app_definition exists and contains required fields (especially
raw.app_definition.nonce) before converting to BigInt, and inside
transformAppDefinitionFromRPC validate/guard raw.nonce (ensure it's defined and
a numeric string/number) and either throw a clear error or provide a safe
default; specifically adjust the appDefinition creation (and/or the
transformAppDefinitionFromRPC function) to validate raw.app_definition.nonce
prior to calling BigInt(raw.nonce) and reference the AppDefinitionV1 required
fields (application, quorum, nonce) when performing the guard.
---
Outside diff comments:
In `@clearnode/api/rpc_router.go`:
- Around line 107-143: The router is missing registration for the new
user.v1.get_action_allowances RPC; add a route binding in the same place as
other user V1 routes by calling
userV1Group.Handle(rpc.UserV1GetActionAllowancesMethod.String(),
userV1Handler.GetActionAllowances) so the userV1Handler's GetActionAllowances
method (and rpc.UserV1GetActionAllowancesMethod) are exposed to clients.
---
Duplicate comments:
In `@clearnode/action_gateway/action_gateway.go`:
- Around line 106-117: The current pattern calling tx.GetUserActionCount and
then tx.RecordAction causes a TOCTOU race under concurrency; replace with an
atomic operation on the transaction layer (e.g., add and call a new
tx.CheckAndRecordAction or tx.IncrementIfBelowLimit) that accepts userAddress,
gatedAction, allowance and defaultTimeWindow and performs the
count-check-and-insert/update within a single DB transaction or row-level lock;
update the caller to use that new method and handle its error/limit return
instead of separately using usedCount and RecordAction.
- Around line 36-47: NewActionGateway currently only checks numeric fields but
must also validate ActionLimitConfig.ActionGates entries: in NewActionGateway
iterate the cfg.ActionGates collection (referencing ActionLimitConfig and
NewActionGateway on ActionGateway), verify each gate key is a recognized/allowed
ID and accumulate a set to ensure no duplicate IDs, and return a descriptive
error if any unknown keys or duplicate IDs are found; update the constructor to
reject the config with an error listing offending keys so invalid or colliding
action_gates cannot be silently accepted.
In `@pkg/core/types.go`:
- Around line 921-924: Two constants collide: GatedActionAppSessionCreation and
GatedActionAppSessionDeposit both map to "app_session_deposit", causing
ambiguous action IDs; update GatedActionAppSessionCreation to a unique literal
(e.g., "app_session_creation") and scan the other GatedAction constants in the
nearby block (lines covering the GatedActionAppSession* group, e.g.,
GatedActionAppSessionOperation, GatedActionAppSessionWithdrawal and any entries
in the 928-956 range) to ensure each has a distinct string value so ID mapping
is unambiguous.
---
Nitpick comments:
In `@cerebro/operator.go`:
- Around line 460-468: The register-app branch currently accepts any third arg
silently; update the case "register-app" handling to explicitly validate the
optional flag: only accept "no-approval" as a valid third argument and set
noApproval accordingly (use args and set noApproval = true only when args[2] ==
"no-approval"); if a third arg exists and is not "no-approval", print an error
message (reusing the existing usage text) and return instead of calling
o.registerApp; keep the call to o.registerApp(ctx, args[1], "", noApproval).
In `@clearnode/api/app_session_v1/handler.go`:
- Around line 35-59: NewHandler should validate required dependencies instead of
accepting nils; change its signature to return (*Handler, error), check that
actionGateway, signer, useStoreInTx, assetStore, stateAdvancer, and statePacker
(and any other mandatory deps) are non-nil, and return a clear error if any are
nil; construct and return the Handler only when validations pass, and update
callers to handle the error.
In `@clearnode/api/app_session_v1/rebalance_app_sessions_test.go`:
- Around line 153-164: The duplicated broad mock for GetApp should be tightened
to assert the exact app IDs being looked up: replace the two
mockStore.On("GetApp", mock.Anything).Return(nil, nil) calls with either a
single mockStore.On("GetApp", expectedAppID).Return(nil, nil).Twice() or two
separate mockStore.On("GetApp", appID1).Return(nil, nil).Once() and
mockStore.On("GetApp", appID2).Return(nil, nil).Once(), using the concrete IDs
used in the test (e.g., sessionID1's app ID and sessionID2's app ID) so the test
validates the lookup arguments; adjust the other occurrences noted (lines around
320-329 and 677-686) similarly and keep the existing UpdateAppSession matcher
intact.
In `@clearnode/api/app_session_v1/testing.go`:
- Around line 146-164: The mock methods GetAppCount, GetTotalUserStaked, and
GetUserActionCount currently do unchecked type assertions on args.Get(0) which
can panic if the mocked call returns nil; update each method
(MockStore.GetAppCount, MockStore.GetTotalUserStaked,
MockStore.GetUserActionCount) to first check if args.Get(0) == nil and if so
return a safe zero value (uint64(0) for counts, decimal.Zero for the decimal)
along with args.Error(1), otherwise perform the type assertion and return the
asserted value plus args.Error(1).
In `@clearnode/api/apps_v1/submit_app_version.go`:
- Around line 52-63: The code lowercases req.App.OwnerWallet when constructing
app.AppV1 but calls h.actionGateway.AllowAppRegistration(tx,
req.App.OwnerWallet) using the raw value, which can cause case-variant behavior;
normalize the owner wallet once (e.g., ownerWallet :=
strings.ToLower(req.App.OwnerWallet)) and then pass that normalized ownerWallet
into AllowAppRegistration and set appEntry.OwnerWallet to the same variable so
both gating (AllowAppRegistration) and persistence (appEntry) use an identical,
lowercased value.
In `@clearnode/api/apps_v1/testing.go`:
- Around line 48-50: The MockStore.GetUserActionCounts stub currently returns
(nil, nil); change it to return an empty map and nil error to avoid forcing nil
checks in tests — specifically update the GetUserActionCounts method on
MockStore to return map[core.GatedAction]uint64{} instead of nil. Keep the
signature and error return the same, only replace the nil map with an
initialized empty map.
In `@clearnode/api/channel_v1/handler.go`:
- Line 16: The constructor NewHandler should validate its actionGateway
parameter and panic with a clear message if it's nil to fail fast; add a
nil-guard in NewHandler that checks the incoming actionGateway (the field
actionGateway on the handler type) and panic("nil actionGateway passed to
NewHandler") or similar, so calls like the one in rpc_router.go that currently
pass through won't cause a later nil dereference in submit_state.go where
h.actionGateway is used.
In `@clearnode/api/user_v1/testing.go`:
- Around line 38-56: The MockStore methods GetAppCount, GetTotalUserStaked,
RecordAction, GetUserActionCount, and GetUserActionCounts currently return
hard-coded zero/nil; update each to delegate to m.Called(...) and return the
configured values and error from the testify/mock call (e.g. ret :=
m.Called(<same args>); extract numeric/map/decimal results with type assertions
and nil-checks, and return ret.Error(1) for the error), taking care to
convert/Get(0) safely for decimal.Decimal (fall back to decimal.Zero if nil) and
maps (return nil if Get(0) is nil) so tests can configure non-zero/stubbed error
cases via m.On(...).Return(...).
In `@pkg/app/app_session_v1.go`:
- Around line 41-52: Add a short explanatory comment to the
AppStateUpdateIntent.GatedAction method clarifying that certain intents (e.g.,
Close and Rebalance) intentionally return an empty GatedAction (ungated) and why
(they do not require gating or do not affect resources protected by gating), so
future readers understand the design decision; update the comment near the
switch or the default case in the GatedAction method of type
AppStateUpdateIntent.
In `@pkg/rpc/client_test.go`:
- Around line 365-372: The test sets nested AppDefinitionV1 but doesn't assert
any of its fields; add assertions after the structure is created to verify
AppDefinitionV1.Application == "0xapp", AppDefinitionV1.Nonce == "1",
AppDefinitionV1.Quorum == 1, that len(AppDefinitionV1.Participants) == 1, and
that the single participant has WalletAddress == testWalletV1 and
SignatureWeight == 1 (check the fields on the rpc.AppDefinitionV1 and
rpc.AppParticipantV1 instances you create).
In `@pkg/rpc/types.go`:
- Around line 150-152: The struct field Application is misnamed relative to its
JSON tag and project convention; rename the Go field to ApplicationID in
pkg/rpc/types.go (update the struct field formerly named Application to
ApplicationID with the existing `json:"application_id"` tag) and then update all
references/usages of Application across the package (functions, constructors,
tests, serialization code) to use ApplicationID to keep API and Go model
consistent.
- Around line 190-191: Rename the struct field in AppSessionInfoV1 from
AppDefinitionV1 AppDefinitionV1 to a clearer identifier (e.g., AppDefinition
AppDefinitionV1) while preserving the json tag `json:"app_definition"` and the
type AppDefinitionV1 to avoid wire-format changes; update all references/usages
of AppSessionInfoV1.AppDefinitionV1 to the new field name (AppDefinition) across
the codebase and run `go build`/tests to catch missing references.
In `@sdk/ts/src/app/types.ts`:
- Line 141: The SDK changed from flat fields to a single appDefinition:
AppDefinitionV1 which is breaking; add a temporary deprecation bridge by keeping
the original flat field names as aliases that forward to appDefinition (either
as read-only getters or computed properties) and mark them with `@deprecated`
JSDoc comments so existing integrators continue to work while encouraging
migration; update the type definition that contains appDefinition (and
associated interfaces) to include these deprecated aliases mapping to the
corresponding appDefinition.<property> values and ensure
serialization/instantiation code (constructors/factory functions) populate both
appDefinition and the deprecated aliases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 886567d2-13e5-47bc-88d4-f2b5c0a65998
📒 Files selected for processing (75)
cerebro/commands.gocerebro/operator.goclearnode/action_gateway/action_gateway.goclearnode/api/app_session_v1/create_app_session.goclearnode/api/app_session_v1/create_app_session_test.goclearnode/api/app_session_v1/get_app_definition.goclearnode/api/app_session_v1/get_app_definition_test.goclearnode/api/app_session_v1/get_app_sessions_test.goclearnode/api/app_session_v1/handler.goclearnode/api/app_session_v1/interface.goclearnode/api/app_session_v1/rebalance_app_sessions.goclearnode/api/app_session_v1/rebalance_app_sessions_test.goclearnode/api/app_session_v1/submit_app_state.goclearnode/api/app_session_v1/submit_app_state_test.goclearnode/api/app_session_v1/submit_deposit_state.goclearnode/api/app_session_v1/submit_deposit_state_test.goclearnode/api/app_session_v1/testing.goclearnode/api/app_session_v1/utils.goclearnode/api/apps_v1/get_apps_test.goclearnode/api/apps_v1/handler.goclearnode/api/apps_v1/interface.goclearnode/api/apps_v1/submit_app_version.goclearnode/api/apps_v1/submit_app_version_test.goclearnode/api/apps_v1/testing.goclearnode/api/channel_v1/get_channels_test.goclearnode/api/channel_v1/get_escrow_channel_test.goclearnode/api/channel_v1/get_home_channel_test.goclearnode/api/channel_v1/get_latest_state_test.goclearnode/api/channel_v1/handler.goclearnode/api/channel_v1/interface.goclearnode/api/channel_v1/request_creation_test.goclearnode/api/channel_v1/submit_state.goclearnode/api/channel_v1/submit_state_test.goclearnode/api/channel_v1/testing.goclearnode/api/metric_store.goclearnode/api/rpc_router.goclearnode/api/user_v1/get_action_allowances.goclearnode/api/user_v1/get_action_allowances_test.goclearnode/api/user_v1/handler.goclearnode/api/user_v1/interface.goclearnode/api/user_v1/testing.goclearnode/chart/templates/configmap.yamlclearnode/chart/values.yamlclearnode/main.goclearnode/runtime.goclearnode/store/database/app_ledger_test.goclearnode/store/database/app_session.goclearnode/store/database/app_session_key_state_test.goclearnode/store/database/app_session_test.goclearnode/store/database/test/postgres_integration_test.goclearnode/store/database/utils.goclearnode/stress/app_session.godocs/api.yamlpkg/app/app_session_v1.gopkg/app/app_session_v1_test.gopkg/core/types.gopkg/rpc/api.gopkg/rpc/client.gopkg/rpc/client_test.gopkg/rpc/error.gopkg/rpc/methods.gopkg/rpc/types.gosdk/go/client_test.gosdk/go/examples/app_sessions/lifecycle.gosdk/go/user.gosdk/go/utils.gosdk/go/utils_test.gosdk/ts/src/app/types.tssdk/ts/src/client.tssdk/ts/src/core/types.tssdk/ts/src/rpc/api.tssdk/ts/src/rpc/client.tssdk/ts/src/rpc/methods.tssdk/ts/src/rpc/types.tssdk/ts/src/utils.ts
d149030 to
1b47760
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
clearnode/store/database/interface.go (1)
214-216:⚠️ Potential issue | 🟡 MinorUpdate stale doc comment to match the method signature.
The comment still mentions “method, and path”, but the function takes
gatedAction.💡 Proposed fix
- // GetUserActionCount returns the number of actions matching the given wallet, method, and path - // within the specified time window. + // GetUserActionCount returns the number of actions matching the given wallet and gated action + // within the specified time window.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clearnode/store/database/interface.go` around lines 214 - 216, Update the stale doc comment for GetUserActionCount to accurately describe its parameters and behavior: mention that it returns the number of actions for the given wallet and gatedAction (type core.GatedAction) within the specified time window (window time.Duration), rather than referring to "method, and path"; ensure the comment describes the parameters wallet, gatedAction, and window and the return values (uint64 count and error) to match the GetUserActionCount signature.clearnode/store/database/user_staked.go (1)
26-36:⚠️ Potential issue | 🟡 MinorTrim wallet input before normalization, and keep read/write validation consistent.
strings.ToLoweralone allows whitespace-only wallet values to pass (e.g.," "), andGetTotalUserStakedcurrently accepts the same untrimmed input path.💡 Proposed fix
func (s *DBStore) UpdateUserStaked(wallet string, blockchainID uint64, amount decimal.Decimal) error { - wallet = strings.ToLower(wallet) + wallet = strings.ToLower(strings.TrimSpace(wallet)) if wallet == "" { return fmt.Errorf("wallet address must not be empty") @@ func (s *DBStore) GetTotalUserStaked(wallet string) (decimal.Decimal, error) { - wallet = strings.ToLower(wallet) + wallet = strings.ToLower(strings.TrimSpace(wallet)) + if wallet == "" { + return decimal.Zero, fmt.Errorf("wallet address must not be empty") + }Also applies to: 60-62
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clearnode/store/database/user_staked.go` around lines 26 - 36, Trim wallet with strings.TrimSpace before calling strings.ToLower and before empty checks so whitespace-only values are rejected; update the current function (the one validating wallet/blockchainID/amount where wallet = strings.ToLower(wallet)) to do wallet = strings.ToLower(strings.TrimSpace(wallet)) and use that normalized value for the empty-string check, and apply the same change to GetTotalUserStaked and the other validation block referenced (the checks at the second occurrence around lines 60-62) so read and write paths have consistent validation for wallet, blockchainID, and amount.IsNegative().clearnode/api/user_v1/get_action_allowances.go (1)
24-27:⚠️ Potential issue | 🟠 MajorUse the transactional store handle inside the tx closure.
Inside the transaction callback,
GetUserAllowancesis called withh.storeinstead oftx, which bypasses the transaction context.💡 Proposed fix
err := h.useStoreInTx(func(tx Store) error { var err error - allowances, err = h.actionGateway.GetUserAllowances(h.store, req.Wallet) + allowances, err = h.actionGateway.GetUserAllowances(tx, req.Wallet) if err != nil { return rpc.Errorf("failed to retrieve action allowances: %w", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clearnode/api/user_v1/get_action_allowances.go` around lines 24 - 27, Inside the useStoreInTx closure you're calling h.actionGateway.GetUserAllowances with h.store which bypasses the transaction; change the call to pass the transactional handle (tx) instead of h.store so GetUserAllowances runs within the transaction context (i.e., inside the closure call in useStoreInTx, invoke h.actionGateway.GetUserAllowances(tx, req.Wallet) and propagate any returned error).clearnode/action_gateway/action_gateway.go (2)
36-54:⚠️ Potential issue | 🟠 MajorFail fast when
action_gatesis empty.An empty/nil
ActionGatesconfig still initializes successfully and effectively disables gating for all actions.💡 Proposed fix
func NewActionGateway(cfg ActionLimitConfig) (*ActionGateway, error) { if !cfg.LevelStepTokens.IsPositive() { return nil, errors.New("LevelStepTokens must be greater than zero") } if !cfg.AppCost.IsPositive() { return nil, errors.New("AppCost must be greater than zero") } + if len(cfg.ActionGates) == 0 { + return nil, errors.New("ActionGates must not be empty") + } seenIDs := make(map[uint8]core.GatedAction, len(cfg.ActionGates))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clearnode/action_gateway/action_gateway.go` around lines 36 - 54, NewActionGateway currently allows cfg.ActionGates to be nil/empty which disables gating; update NewActionGateway to validate cfg.ActionGates is non-nil and non-empty and return an error when it's empty (e.g., "ActionGates must contain at least one gated action"). Locate the constructor NewActionGateway and the use of cfg.ActionGates/seenIDs and add a pre-check that len(cfg.ActionGates) > 0 (and ensure cfg.ActionGates != nil) before building seenIDs so the function fails fast on missing action_gates.
118-129:⚠️ Potential issue | 🔴 Critical
AllowActionstill has a check-then-write race.
GetUserActionCount+RecordActionis non-atomic. Concurrent requests for the same(wallet, action)can both pass the check and exceed the configured allowance.💡 Suggested direction
type Store interface { - RecordAction(wallet string, gatedAction core.GatedAction) error - GetUserActionCount(wallet string, gatedAction core.GatedAction, window time.Duration) (uint64, error) + TryConsumeAction(wallet string, gatedAction core.GatedAction, window time.Duration, allowance uint64) error } - usedCount, err := tx.GetUserActionCount(userAddress, gatedAction, defaultTimeWindow) - if err != nil { - return fmt.Errorf("failed to get user action count: %w", err) - } - if usedCount >= allowance { - return fmt.Errorf("action %s limit reached: used %d of %d allowed in 24h", gatedAction, usedCount, allowance) - } - if err := tx.RecordAction(userAddress, gatedAction); err != nil { - return fmt.Errorf("failed to record action: %w", err) - } + if err := tx.TryConsumeAction(userAddress, gatedAction, defaultTimeWindow, allowance); err != nil { + return fmt.Errorf("failed to consume action allowance: %w", err) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clearnode/action_gateway/action_gateway.go` around lines 118 - 129, AllowAction currently does a non-atomic check-then-write using tx.GetUserActionCount followed by tx.RecordAction, which allows a race where concurrent requests both pass the check; fix by performing the check-and-increment atomically in the datastore (e.g., implement and call a single method like tx.IncrementAndCheckUserAction or tx.RecordAndCheckAction that performs a SQL transaction/SELECT ... FOR UPDATE or an atomic counter update and returns whether the increment is allowed), then replace the GetUserActionCount+RecordAction sequence in AllowAction with that single atomic call and handle its returned error/limit result accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cerebro/operator.go`:
- Around line 477-484: The register-app handler currently treats any third
argument as false (approval required) which hides typos like "no_aproval";
update the case "register-app" code to strictly validate args[2] so only the
exact string "no-approval" is accepted and any other non-empty third argument
produces a clear error/usage message; adjust the logic around noApproval and the
call to o.registerApp(ctx, args[1], "", noApproval) to only pass true when
args[2] == "no-approval" and return with an error print if args[2] is present
but not exactly that value.
In `@clearnode/store/database/user_staked.go`:
- Line 15: The Amount field on the UserStaked model currently uses
`gorm:"column:amount;type:varchar(78);not null"` which diverges from Postgres
migration; update the GORM tag on the Amount field in
clearnode/store/database/user_staked.go to `type:numeric(78,18)` (keeping column
name and not null) so SQLite AutoMigrate matches Postgres NUMERIC(78,18); ensure
this change is reflected where the Amount field is referenced (e.g.,
GetTotalUserStaked aggregation) to avoid type mismatch during SUM(amount)
queries.
In `@docs/api.yaml`:
- Around line 159-161: The TS SDK still expects/emits the old "application"
field which mismatches the wire "application_id": update AppDefinitionV1 type
(sdk/ts/src/app/types.ts) to use application_id: string, change
transformAppDefinitionFromRPC (sdk/ts/src/utils.ts) to read raw.application_id
(and validate presence of application_id and nonce) and adjust its error message
accordingly, and change transformAppDefinitionToRPC to emit application_id
instead of application in the RPC payload; ensure any other references to
AppDefinitionV1.application are renamed to application_id so inbound parsing
(getAppDefinition) and outbound creation (createAppSession) align with the Go
JSON tag.
---
Duplicate comments:
In `@clearnode/action_gateway/action_gateway.go`:
- Around line 36-54: NewActionGateway currently allows cfg.ActionGates to be
nil/empty which disables gating; update NewActionGateway to validate
cfg.ActionGates is non-nil and non-empty and return an error when it's empty
(e.g., "ActionGates must contain at least one gated action"). Locate the
constructor NewActionGateway and the use of cfg.ActionGates/seenIDs and add a
pre-check that len(cfg.ActionGates) > 0 (and ensure cfg.ActionGates != nil)
before building seenIDs so the function fails fast on missing action_gates.
- Around line 118-129: AllowAction currently does a non-atomic check-then-write
using tx.GetUserActionCount followed by tx.RecordAction, which allows a race
where concurrent requests both pass the check; fix by performing the
check-and-increment atomically in the datastore (e.g., implement and call a
single method like tx.IncrementAndCheckUserAction or tx.RecordAndCheckAction
that performs a SQL transaction/SELECT ... FOR UPDATE or an atomic counter
update and returns whether the increment is allowed), then replace the
GetUserActionCount+RecordAction sequence in AllowAction with that single atomic
call and handle its returned error/limit result accordingly.
In `@clearnode/api/user_v1/get_action_allowances.go`:
- Around line 24-27: Inside the useStoreInTx closure you're calling
h.actionGateway.GetUserAllowances with h.store which bypasses the transaction;
change the call to pass the transactional handle (tx) instead of h.store so
GetUserAllowances runs within the transaction context (i.e., inside the closure
call in useStoreInTx, invoke h.actionGateway.GetUserAllowances(tx, req.Wallet)
and propagate any returned error).
In `@clearnode/store/database/interface.go`:
- Around line 214-216: Update the stale doc comment for GetUserActionCount to
accurately describe its parameters and behavior: mention that it returns the
number of actions for the given wallet and gatedAction (type core.GatedAction)
within the specified time window (window time.Duration), rather than referring
to "method, and path"; ensure the comment describes the parameters wallet,
gatedAction, and window and the return values (uint64 count and error) to match
the GetUserActionCount signature.
In `@clearnode/store/database/user_staked.go`:
- Around line 26-36: Trim wallet with strings.TrimSpace before calling
strings.ToLower and before empty checks so whitespace-only values are rejected;
update the current function (the one validating wallet/blockchainID/amount where
wallet = strings.ToLower(wallet)) to do wallet =
strings.ToLower(strings.TrimSpace(wallet)) and use that normalized value for the
empty-string check, and apply the same change to GetTotalUserStaked and the
other validation block referenced (the checks at the second occurrence around
lines 60-62) so read and write paths have consistent validation for wallet,
blockchainID, and amount.IsNegative().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5cf49893-8ae9-4702-b8ae-6bd3b3531885
📒 Files selected for processing (31)
cerebro/commands.gocerebro/operator.goclearnode/action_gateway/action_gateway.goclearnode/api/app_session_v1/rebalance_app_sessions.goclearnode/api/app_session_v1/rebalance_app_sessions_test.goclearnode/api/app_session_v1/submit_app_state.goclearnode/api/app_session_v1/submit_app_state_test.goclearnode/api/app_session_v1/submit_deposit_state.goclearnode/api/app_session_v1/submit_deposit_state_test.goclearnode/api/app_session_v1/testing.goclearnode/api/apps_v1/submit_app_version.goclearnode/api/apps_v1/testing.goclearnode/api/channel_v1/testing.goclearnode/api/user_v1/get_action_allowances.goclearnode/api/user_v1/get_action_allowances_test.goclearnode/api/user_v1/testing.goclearnode/chart/config/prod/clearnode.yamlclearnode/chart/config/sandbox/clearnode.yamlclearnode/chart/config/uat/clearnode.yamlclearnode/chart/config/v1-rc/clearnode.yamlclearnode/chart/values.yamlclearnode/config/schemas/action_gateway_schema.yamlclearnode/store/database/action_log.goclearnode/store/database/app_session.goclearnode/store/database/interface.goclearnode/store/database/user_staked.godocs/api.yamlpkg/core/types.gosdk/go/user.gosdk/ts/src/rpc/types.tssdk/ts/src/utils.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- clearnode/config/schemas/action_gateway_schema.yaml
- clearnode/chart/values.yaml
- sdk/ts/src/utils.ts
- clearnode/store/database/action_log.go
- sdk/ts/src/rpc/types.ts
- clearnode/api/app_session_v1/submit_app_state.go
- clearnode/api/user_v1/testing.go
- clearnode/api/app_session_v1/rebalance_app_sessions.go
Comment on lines
+477
to
+484
| case "register-app": | ||
| if len(args) < 2 { | ||
| fmt.Println("ERROR: Usage: register-app <app_id> [no-approval]") | ||
| fmt.Println("INFO: Pass 'no-approval' as second arg to allow session creation without owner approval") | ||
| return | ||
| } | ||
| noApproval := len(args) >= 3 && args[2] == "no-approval" | ||
| o.registerApp(ctx, args[1], "", noApproval) |
Contributor
There was a problem hiding this comment.
Validate register-app optional flag strictly.
At Line 483, unknown third arguments are silently accepted and treated as approval-required mode. A typo (for example, no_aproval) will register with the wrong policy without warning.
Suggested fix
case "register-app":
if len(args) < 2 {
fmt.Println("ERROR: Usage: register-app <app_id> [no-approval]")
fmt.Println("INFO: Pass 'no-approval' as second arg to allow session creation without owner approval")
return
}
- noApproval := len(args) >= 3 && args[2] == "no-approval"
+ if len(args) >= 3 && args[2] != "no-approval" {
+ fmt.Println("ERROR: Usage: register-app <app_id> [no-approval]")
+ return
+ }
+ noApproval := len(args) >= 3
o.registerApp(ctx, args[1], "", noApproval)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cerebro/operator.go` around lines 477 - 484, The register-app handler
currently treats any third argument as false (approval required) which hides
typos like "no_aproval"; update the case "register-app" code to strictly
validate args[2] so only the exact string "no-approval" is accepted and any
other non-empty third argument produces a clear error/usage message; adjust the
logic around noApproval and the call to o.registerApp(ctx, args[1], "",
noApproval) to only pass true when args[2] == "no-approval" and return with an
error print if args[2] is present but not exactly that value.
| type UserStakedV1 struct { | ||
| UserWallet string `gorm:"column:user_wallet;primaryKey;not null"` | ||
| BlockchainID uint64 `gorm:"column:blockchain_id;primaryKey;not null"` | ||
| Amount decimal.Decimal `gorm:"column:amount;type:varchar(78);not null"` |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== UserStakedV1 model tag =="
rg -n --context 3 'type UserStakedV1|Amount\s+decimal\.Decimal' clearnode/store/database/user_staked.go || echo "No match found"
echo
echo "== Postgres DDL for user_staked_v1 =="
rg -n --context 8 'CREATE TABLE user_staked_v1|amount NUMERIC' clearnode/config/migrations/postgres/20251222000000_initial_schema.sql || echo "No match found"
echo
echo "== SQLite migration path =="
rg -n --context 2 'UserStakedV1' clearnode/store/database/database.go || echo "No match found"
echo
echo "== Check for GetTotalUserStaked function =="
rg -n 'GetTotalUserStaked' clearnode/ || echo "No match found"Repository: layer-3/nitrolite
Length of output: 5272
Change amount column type from varchar(78) to numeric(78,18) in the GORM model tag.
The model defines type:varchar(78) but the Postgres migration specifies NUMERIC(78, 18). This causes SQLite (which uses AutoMigrate with the model definition) to diverge from Postgres. The GetTotalUserStaked function performs SUM(amount) aggregation, making this type mismatch problematic for cross-database consistency.
Proposed fix
- Amount decimal.Decimal `gorm:"column:amount;type:varchar(78);not null"`
+ Amount decimal.Decimal `gorm:"column:amount;type:numeric(78,18);not null"`📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Amount decimal.Decimal `gorm:"column:amount;type:varchar(78);not null"` | |
| Amount decimal.Decimal `gorm:"column:amount;type:numeric(78,18);not null"` |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clearnode/store/database/user_staked.go` at line 15, The Amount field on the
UserStaked model currently uses `gorm:"column:amount;type:varchar(78);not null"`
which diverges from Postgres migration; update the GORM tag on the Amount field
in clearnode/store/database/user_staked.go to `type:numeric(78,18)` (keeping
column name and not null) so SQLite AutoMigrate matches Postgres NUMERIC(78,18);
ensure this change is reflected where the Amount field is referenced (e.g.,
GetTotalUserStaked aggregation) to avoid type mismatch during SUM(amount)
queries.
4cf4d2a to
613350b
Compare
philanton
approved these changes
Mar 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Configuration
Breaking Changes