Skip to content

Feat: small refactoring#13

Open
mrLSD wants to merge 3 commits intomainfrom
feat/mrlsd/small-refactoring
Open

Feat: small refactoring#13
mrLSD wants to merge 3 commits intomainfrom
feat/mrlsd/small-refactoring

Conversation

@mrLSD
Copy link
Copy Markdown
Member

@mrLSD mrLSD commented Oct 3, 2025

➡️ gas calculation without saturation to avoid potential silent overflow
➡️ minor refactoring of functions

Summary by CodeRabbit

  • Refactor

    • Internal calculations and result-handling logic were simplified and made more functional-style; behavior and outcomes are unchanged.
  • Notes

    • No user-facing changes; existing functionality, outputs, and error behavior remain the same.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Oct 3, 2025

Walkthrough

Refactors internal computations and control-flow style in solver-registry: lib.rs constructs GAS_REMOVE_WORKER_KEY_CALLBACK via Gas::from_gas summing components; pool.rs replaces explicit error-branching with Result::ok().map and a match in two functions. No public API changes.

Changes

Cohort / File(s) Summary
Gas computation refactor
contracts/solver-registry/src/lib.rs
Replace chained saturating_add of multiple gas constants with Gas::from_gas using an explicit sum of Gas components. Total gas unchanged.
Functional control-flow mapping in pool ops
contracts/solver-registry/src/pool.rs
- on_create_liquidity_pool_account: replace explicit is_err/else branching with Result::ok().map(...) to return Some(pool_id) on Ok, None on Err.
- on_deposit_into_pool: replace if let Ok(used_fund) with match to return U128(amount - used) on Ok, amount on Err. Behavior unchanged; no API changes.

Sequence Diagram(s)

sequenceDiagram
  participant Caller
  participant Lib as lib.rs
  participant Pool as pool.rs

  note right of Lib #DDEBF7: GAS constant construction (pure computation)
  Caller->>Lib: reference GAS_REMOVE_WORKER_KEY_CALLBACK
  Lib-->>Caller: Gas::from_gas(sum of components)

  Caller->>Pool: on_create_liquidity_pool_account(...)
  Pool->>Pool: underlying create -> Result
  alt Ok
    Pool-->>Caller: Some(pool_id)    /* via Result::ok().map */
  else Err
    Pool-->>Caller: None
  end

  Caller->>Pool: on_deposit_into_pool(amount,...)
  Pool->>Pool: underlying deposit -> Result<used, Err>
  alt Ok
    Pool-->>Caller: U128(amount - used)
  else Err
    Pool-->>Caller: amount
  end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

Little rabbit hops through lines of code,
I sum the gas where numbers flowed.
I map the results with a cheerful twitch,
Match the branches, tidy the switch.
Hooray—no API lost, just neater mode. 🥕🧡

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Title Check ❓ Inconclusive The title “Feat: small refactoring” is overly generic and does not highlight the main changes introduced in the pull request. It fails to mention the key update of replacing saturating gas calculation to avoid silent overflow or the specific functional refactoring in pool methods. A more descriptive title would help reviewers quickly understand the primary impact. It's currently too vague to serve as an effective summary. Update the title to reflect the core refactor, for example “Refactor gas calculation to prevent silent overflow and simplify pool method error handling.” This provides clear context on the main changes and intent. A descriptive title will improve clarity for future reviewers scanning the commit history.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat/mrlsd/small-refactoring

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 47b71a0 and e186daf.

📒 Files selected for processing (1)
  • contracts/solver-registry/src/lib.rs (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • contracts/solver-registry/src/lib.rs
⏰ 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). (2)
  • GitHub Check: Build Solver Registry
  • GitHub Check: Tests

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

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3db2244 and aa1bf6b.

📒 Files selected for processing (2)
  • contracts/solver-registry/src/lib.rs (1 hunks)
  • contracts/solver-registry/src/pool.rs (3 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). (2)
  • GitHub Check: Build Solver Registry
  • GitHub Check: Tests
🔇 Additional comments (2)
contracts/solver-registry/src/pool.rs (2)

122-136: LGTM! Clearer functional-style control flow.

The transformation from imperative if/else to ok().map() correctly preserves behavior:

  • Ok(_) → executes the block and returns Some(pool_id)
  • Err(_) → returns None

This makes the Result-to-Option transformation more explicit.


145-150: LGTM! More explicit Result handling.

The transformation from if let Ok to match makes both Result branches explicit while preserving behavior:

  • Ok(used) → returns refund amount U128(amount - used)
  • Err(_) → returns full amount (no refund)

Note: saturating_sub is appropriately retained here to guard against underflow in refund calculation, which is a different use case than the gas calculation saturation mentioned in the PR objectives.

@mrLSD mrLSD requested a review from aleksuss October 3, 2025 09:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant