perf(server): reuse SuiClient across requests

[dongowu]

1. crates/server/src/server.rs:102 Store rpc_url in AppState and use tokio::sync::OnceCell to lazily initialize and reuse
   a single sui_sdk::SuiClient.
2. crates/server/src/server.rs:130 Add AppState::sui_client() and replace per-request SuiClientBuilder::build(...) calls with the cached client.
3. crates/server/src/server.rs:225 Simplify Axum state from (Arc, Url) to Arc, and update status, orderbook, deep_supply, and summary to use the cached client.

[sb-]

@claude

[claude]

Code Review: PR #764 - Reuse SuiClient across requests

Summary

This PR introduces a performance optimization by caching and reusing a single SuiClient instance across all requests instead of creating a new client for each request. Overall, this is a solid improvement with good implementation quality.

✅ Strengths

1. Performance Improvement: Reusing the SuiClient is the correct approach and will significantly reduce overhead from repeatedly establishing connections and building clients.

2. Clean Implementation: The use of tokio::sync::OnceCell is appropriate for lazy initialization with async code and provides thread-safe access.

3. Simplified API Surface: Removing the Url from the Axum state tuple simplifies the state management and makes the code cleaner.

4. Good Error Handling: The sui_client() method properly propagates errors using map_err(DeepBookError::from), and the error conversion is already implemented via the From<sui_sdk::error::Error> trait.

5. Consistent Pattern: All four endpoint functions (status, orderbook, deep_supply, summary) have been updated consistently.

🔍 Code Quality & Best Practices

Good:

• Clear commit message following conventional commits format
• Minimal, focused changes (30 additions, 13 deletions)
• No unnecessary refactoring or scope creep

🐛 Potential Issues

Minor - Documentation:

• Consider adding a doc comment to the sui_client() method explaining that it lazily initializes and caches the client:

  /// Returns a reference to the shared SuiClient instance.
  /// Lazily initializes the client on first access and caches it for subsequent calls.
  pub async fn sui_client(&self) -> Result<&sui_sdk::SuiClient, DeepBookError>

Minor - Initialization Timing:

• The current implementation delays client initialization until first use. While this is fine for most cases, if the RPC URL is invalid, the error won't surface until the first API request rather than at server startup. Consider whether eager initialization might be preferable for fail-fast behavior:

  // In AppState::new(), optionally:
  let sui_client = Arc::new(OnceCell::new());
  sui_client.get_or_try_init(|| async {
      SuiClientBuilder::default().build(rpc_url.as_str()).await
  }).await?;

  This is a design choice and the current lazy approach is valid—just worth considering.

🔒 Security Considerations

No issues identified. The changes don't introduce security concerns:

• The OnceCell provides thread-safe access
• Error handling preserves existing security model
• No new user input or validation paths introduced

⚡ Performance Considerations

Excellent improvement:

• Eliminates repeated client construction overhead
• Reduces connection establishment overhead
• The OnceCell adds minimal overhead (just an atomic check after initialization)

Note: The Arc<OnceCell<SuiClient>> pattern is efficient, but be aware that all requests share the same client. This is generally fine for the Sui SDK, but ensure that:

• The SuiClient is designed for concurrent use (it is)
• Rate limiting, if any, is handled appropriately at the RPC level

🧪 Test Coverage

Gap identified:

• No tests were added for this functionality
• No existing test files found in crates/server/

Recommendations:

1. Add integration tests to verify the client is actually reused
2. Add error handling tests for client initialization failures
3. Consider adding a test that verifies concurrent requests use the same client instance

Example test structure:

#[cfg(test)]
mod tests {
    use super::*;

    #[tokio::test]
    async fn test_sui_client_reuse() {
        // Verify multiple calls return the same client instance
    }

    #[tokio::test]
    async fn test_sui_client_initialization_error() {
        // Verify proper error handling for invalid RPC URLs
    }
}

📋 Recommendations

Priority: Low

1. Add doc comment to sui_client() method
2. Consider whether eager vs. lazy initialization is preferred for your use case
3. Add tests to verify the caching behavior and error handling

Priority: Optional

• Consider adding a metric/log on first client initialization to track startup behavior

✨ Conclusion

This is a well-implemented performance optimization with no critical issues. The code is clean, follows Rust best practices, and provides meaningful performance benefits. The main gap is test coverage, but the implementation itself is solid.

Recommendation: Approve with optional improvements for tests and documentation.

Great work! 🚀

[dongowu]

@sb- hi please review this pr

[dongowu]

@sb- the ci of rustfmt have some error, I have fix that,please review again. thanks

[dongowu]

@sb- no merge？

[sb-]

Please resolve the merge conflicts