Skip to content
This repository was archived by the owner on Apr 25, 2026. It is now read-only.

fix: update credits rpc and constants#1045

Merged
1xstj merged 2 commits into
mainfrom
1xstj/enable-mainnet-rpc
Aug 4, 2025
Merged

fix: update credits rpc and constants#1045
1xstj merged 2 commits into
mainfrom
1xstj/enable-mainnet-rpc

Conversation

@1xstj
Copy link
Copy Markdown
Contributor

@1xstj 1xstj commented Aug 4, 2025

Summary of changes
Changes introduced in this pull request:

Reference issue to close (if applicable)

Closes

@1xstj 1xstj marked this pull request as ready for review August 4, 2025 19:39
@1xstj 1xstj requested a review from shekohex as a code owner August 4, 2025 19:39
@1xstj 1xstj merged commit 47ef523 into main Aug 4, 2025
8 checks passed
@1xstj 1xstj deleted the 1xstj/enable-mainnet-rpc branch August 4, 2025 19:39
@claude
Copy link
Copy Markdown

claude Bot commented Aug 4, 2025

Claude finished @1xstj's task —— View job


PR Review: Update Credits RPC and Constants

Comprehensive Code Review Analysis

After examining the codebase, I'm providing feedback on the credits RPC implementation and related constants. While this appears to be an initial repository commit, I'll focus on the credits-related components as indicated by the PR title.

🔍 Code Quality & Best Practices

Positive Aspects:

  • Well-documented pallet: The credits pallet has excellent documentation explaining the design philosophy and integration points
  • Type safety: Proper use of Substrate's type system with bounded vectors and parameter traits
  • Modular design: Clean separation between RPC layer, runtime API, and pallet logic
  • Error handling: Proper error propagation in RPC methods

Areas for Improvement:

1. RPC Error Handling (pallets/credits/rpc/src/lib.rs:91-96)
match api.query_user_credits(at, account_id) {
    Ok(Ok(res)) => Ok(res),
    Ok(Err(e)) => Err(map_err(format!("{:?}", e), "Unable to query user credits")),
    Err(e) => Err(map_err(format!("{:?}", e), "Unable to query user credits")),
}

Issue: Both error cases use the same error message, making debugging difficult.
Recommendation: Differentiate between runtime API errors and dispatch errors.

2. Constants Documentation (primitives/src/lib.rs:314-315)
/// The maximum rate per block for stake tiers
/// 1 TNT per block = 14,400 TNT per day
pub const MAX_RATE_PER_BLOCK: Balance = UNIT;

Issue: The comment suggests this allows 1 TNT per block, but doesn't explain the economic rationale.
Recommendation: Add more context about why this specific rate was chosen and its economic implications.

🐛 Potential Bugs & Issues

1. Integer Overflow Risk (pallets/credits/src/lib.rs:75)

The BalanceOf<T> type alias doesn't include explicit overflow protection. Consider using saturating arithmetic throughout calculations.

2. RPC Method Naming (pallets/credits/rpc/src/lib.rs:44,51)

#[method(name = "credits_queryUserCredits")]
#[method(name = "credits_queryUserCreditsWithAsset")]

Issue: Method names don't follow camelCase convention typically used in JSON-RPC.
Recommendation: Consider credits_queryUserCreditscredits_getUserCredits

Performance Considerations

1. Storage Efficiency

  • AssetStakeTiers uses OptionQuery which is efficient for sparse data
  • LastRewardUpdateBlock uses Blake2_128Concat hasher - appropriate for account-based lookups
  • Bounded vectors prevent unbounded growth ✅

2. RPC Performance

  • Direct runtime API calls without caching could be expensive for frequently called methods
  • Consider implementing caching for user credit queries if they become performance bottlenecks

🔒 Security Implications

1. Access Control

  • ForceOrigin properly restricts stake tier updates to authorized origins
  • No direct balance mutations through RPC (read-only) ✅

2. Input Validation

  • RPC methods accept external AccountId and AssetId parameters
  • Runtime API should validate these inputs to prevent potential issues

3. Credit Window Logic

type ClaimWindowBlocks: Get<BlockNumberOf<Self>>;

The 7-day claim window (DAYS * 7) could lead to:

  • Users losing credits if they don't claim within the window
  • Consider implementing warnings or notifications for approaching deadlines

📖 Documentation Updates Needed

1. Missing RPC Documentation

The RPC methods lack comprehensive documentation for:

  • Expected parameter formats
  • Return value structures
  • Error conditions and codes
  • Usage examples

2. Integration Guide

Missing documentation on:

  • How to set up the credits pallet in a runtime
  • Configuration examples for different network scenarios
  • Migration paths for existing deployments

🏗️ Architecture & Design

Strengths:

  • Clear separation of concerns between event emission and actual credit management
  • Flexible asset-specific stake tiers
  • Integration with multi-asset delegation system

Potential Improvements:

  1. Event Structure: Consider adding more metadata to events for better off-chain processing
  2. Rate Limiting: No built-in rate limiting for credit queries - consider adding this for production

🎯 Recommendations Summary

High Priority:

  1. Improve RPC error differentiation for better debugging
  2. Add comprehensive RPC documentation
  3. Review integer overflow protection in calculations

Medium Priority:

  1. Consider RPC method naming conventions
  2. Add economic rationale documentation for constants
  3. Implement query caching for performance

Low Priority:

  1. Add integration examples and migration guides
  2. Consider rate limiting for production deployments

Overall Assessment

The credits RPC implementation demonstrates solid Substrate development practices with good type safety and modular design. The code is well-structured and follows Substrate conventions. The main areas for improvement are around error handling specificity, documentation completeness, and performance considerations for production use.

Security: ✅ No major security concerns identified
Functionality: ✅ Implements intended credit tracking features
Code Quality: ⚠️ Good overall, with room for improvement in error handling and documentation

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants