Skip to content

Conversation

Marcofann
Copy link
Contributor

@Marcofann Marcofann commented Aug 21, 2025

What

  • aminojson: Validate Duration in marshalDuration
    • Bounds-check seconds (±MaxDurationSeconds) to avoid int64 overflow/underflow
    • Enforce nanos ∈ [-999999999, 999999999]
    • Require consistent signs between seconds and nanos
  • simulation: Document transition matrix usage
    • Add usage example to CreateTransitionMatrix
    • Explain how default matrices are used in Params
  • tests(aminojson): Add unit tests for marshalDuration validations

Tests

  • time_test.go: valid case, invalid nanos range, sign mismatch, seconds overflow/underflow

Copy link
Contributor

coderabbitai bot commented Aug 21, 2025

📝 Walkthrough

Walkthrough

Adds default liveness and block-size transition matrices and wires them into Params.RandomParams. Expands documentation for transition matrices and their usage. Implements protobuf Duration validation in marshalDuration, adding range and sign checks before computing total nanoseconds.

Changes

Cohort / File(s) Summary
Simulation: transition matrices and docs
x/simulation/params.go, x/simulation/transition_matrix.go
Added unexported default transition matrices created via CreateTransitionMatrix and assigned in RandomParams; expanded documentation explaining matrix semantics and usage; no API/signature changes.
Amino JSON time: Duration validation
x/tx/signing/aminojson/time.go
Implemented protobuf Duration validation: seconds bounds check, nanos range check, and sign consistency check; early-return errors on invalid input; no API/signature changes.

Sequence Diagram(s)

sequenceDiagram
  actor Caller
  participant Time as marshalDuration
  Caller->>Time: marshalDuration(seconds, nanos)
  alt Invalid range/sign
    Time-->>Caller: error (range or sign violation)
  else Valid
    Time->>Time: compute totalNanos
    Time-->>Caller: encoded duration
  end
  note right of Time: Validations: seconds bounds, nanos in [-999,999,999], sign consistency
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Tip

🔌 Remote MCP (Model Context Protocol) integration is now available!

Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats.


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between 7082f88 and ce5b757.

📒 Files selected for processing (3)
  • x/simulation/params.go (1 hunks)
  • x/simulation/transition_matrix.go (1 hunks)
  • x/tx/signing/aminojson/time.go (1 hunks)
⏰ 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: Summary
🔇 Additional comments (7)
x/simulation/transition_matrix.go (1)

23-41: Excellent documentation improvement with comprehensive usage example!

The new documentation block provides a clear, practical example showing how to construct a transition matrix, handle errors, and use the NextState method. This addresses the missing documentation and makes the API much more accessible to developers.

x/tx/signing/aminojson/time.go (3)

61-67: Robust bounds validation for seconds field.

The validation correctly prevents int64 overflow/underflow when converting seconds to nanoseconds. The use of MaxDurationSeconds constant and symmetric checks for both positive and negative bounds is well-implemented.


75-78: Proper nanos range validation according to protobuf specifications.

The validation correctly enforces the protobuf Duration constraint that nanos must be in the range [-999,999,999, 999,999,999]. The error message is clear and informative.


79-82: Essential sign consistency validation.

This validation prevents invalid Duration representations where seconds and nanos have opposite signs, which would be semantically incorrect for a duration value. This is a critical validation that ensures data integrity.

x/simulation/params.go (3)

23-36: Comprehensive documentation for transition matrices.

The documentation block clearly explains the purpose and structure of both transition matrices, including the column-as-current-state and row-as-next-state convention. The explanation of how CreateTransitionMatrix precomputes totals for efficiency is valuable context.


96-97: Proper integration of default matrices into RandomParams.

The matrices are correctly wired into the Params structure, ensuring that simulations will use these well-defined transition behaviors for liveness and block size modeling.


40-53: Safe to discard CreateTransitionMatrix errors for hard-coded square matrices

CreateTransitionMatrix only returns an error if the provided matrix isn’t square. Both defaultLivenessTransitionMatrix and defaultBlockSizeTransitionMatrix in x/simulation/params.go (lines 40–53) are explicit 3×3 literals, so the blank identifier _ safely discards the error here. No changes required.

✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

‼️ IMPORTANT
Auto-reply has been disabled for this repository in the CodeRabbit settings. The CodeRabbit bot will not respond to your replies unless it is explicitly tagged.

  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@Marcofann Marcofann changed the title Validate duration in aminojson; add transition matrix docs chore: validate duration in aminojson; add transition matrix docs Aug 21, 2025
@Marcofann
Copy link
Contributor Author

@aljo242

I’ve added unit tests for marshalDuration in x/tx/signing/aminojson/time_test.go covering:

  • invalid nanos range ([-999999999, 999999999])
  • seconds/nanos sign mismatch
  • seconds overflow/underflow vs MaxDurationSeconds
  • a valid happy-path case

All tests pass locally

@Marcofann Marcofann changed the title chore: validate duration in aminojson; add transition matrix docs chore: validate Duration; simulation: document transition matrix; add tests Aug 22, 2025
@aljo242
Copy link
Contributor

aljo242 commented Aug 22, 2025

These seem like two separate PRs that should be their own PRs

@aljo242 aljo242 closed this Aug 22, 2025
@Marcofann
Copy link
Contributor Author

@aljo242 — I split the changes into two separate PRs as suggested:

  • fix: validate Duration in marshalDuration; add unit tests — #25256
  • docs: document transition matrix usage; add example — #25255

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants