Skip to content

feat(walrus-sui): skip JSON-RPC client when fully migrated to gRPC #3186

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
nikos-terzo wants to merge 1 commit into
base: main
Choose a base branch
from
+23 −6
Draft

feat(walrus-sui): skip JSON-RPC client when fully migrated to gRPC #3186

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 15 additions & 6 deletions crates/walrus-sui/src/client/dual_client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ use tonic::service::interceptor::InterceptedService;
use walrus_core::ensure;

use crate::{
client::SuiClientError,
client::{SuiClientError, retry_client::retriable_sui_client::GrpcMigrationLevel},
coin::Coin,
Comment on lines 38 to 40
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

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

Importing GrpcMigrationLevel from retry_client::retriable_sui_client makes dual_client depend on the retry-layer module that already depends on DualClient, increasing coupling and risking cyclic-module tangles. Consider moving GrpcMigrationLevel (and its env parsing) into a lower-level/shared module (e.g. under client/ root) and re-exporting it, so both DualClient and retry code depend on the same shared definition.

Copilot uses AI. Check for mistakes.
contracts::{AssociatedContractStruct, TypeOriginMap},
};
Expand Down Expand Up @@ -96,17 +96,26 @@ pub(crate) struct CoinBatch {

impl DualClient {
/// Create a new DualClient with the given RPC URL and optional request timeout.
///
/// When `WALRUS_GRPC_MIGRATION_LEVEL` is at its maximum, the JSON-RPC SuiClient is not
/// created. This allows connecting to gRPC-only nodes (e.g. sui-forking).
pub async fn new(
rpc_url: impl AsRef<str>,
request_timeout: Option<Duration>,
) -> Result<Self, SuiClientError> {
let mut client_builder = SuiClientBuilder::default();
if let Some(request_timeout) = request_timeout {
client_builder = client_builder.request_timeout(request_timeout);
}
let rpc_url = rpc_url.as_ref();
let sui_client = Some(client_builder.build(rpc_url).await?);
let grpc_client = GrpcClient::new(rpc_url).context("unable to create grpc client")?;

let sui_client = if GrpcMigrationLevel::default().is_fully_migrated() {
None
} else {
let mut client_builder = SuiClientBuilder::default();
if let Some(request_timeout) = request_timeout {
client_builder = client_builder.request_timeout(request_timeout);
}
Some(client_builder.build(rpc_url).await?)
};
Comment on lines +109 to +117
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

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

DualClient::new() can now set sui_client to None when WALRUS_GRPC_MIGRATION_LEVEL >= 4, but there are still call paths in the crate that unconditionally call DualClient::sui_client() (which panics when sui_client is None), e.g. RetriableSuiClient::query_transaction_blocks / get_owned_objects. This turns a config change into a runtime panic. Consider either (a) only skipping JSON-RPC client creation once all JSON-RPC-dependent methods are gated off at that migration level, or (b) making JSON-RPC client creation lazy / fallible at the point of use and returning a structured error instead of panicking.

Copilot uses AI. Check for mistakes.

Ok(Self {
sui_client,
grpc_client,
Expand Down
8 changes: 8 additions & 0 deletions crates/walrus-sui/src/client/retry_client/retriable_sui_client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,14 @@ const GRPC_MIGRATION_LEVEL_BATCH_OBJECTS: GrpcMigrationLevel = GrpcMigrationLeve
const GRPC_MIGRATION_LEVEL_SELECT_COINS: GrpcMigrationLevel = GrpcMigrationLevel(3);
const GRPC_MIGRATION_LEVEL_GET_BALANCE: GrpcMigrationLevel = GrpcMigrationLevel(4);

impl GrpcMigrationLevel {
/// Returns true if the migration level is high enough that all RPC calls use gRPC
/// and the JSON-RPC SuiClient is not needed.
Comment on lines +216 to +217
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

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

is_fully_migrated() is documented as meaning "all RPC calls use gRPC and the JSON-RPC SuiClient is not needed", but the threshold used (GRPC_MIGRATION_LEVEL_GET_BALANCE) only reflects migration of balance retrieval. There are still JSON-RPC-only methods in the codebase (e.g. transaction queries / owned-object listing), so this helper name/doc are misleading and (when used to skip client creation) can trigger panics later. Consider renaming/rewording this helper to match what level 4 actually guarantees, or updating the threshold/levels so that "fully migrated" truly implies no JSON-RPC usage.

Suggested change
/// Returns true if the migration level is high enough that all RPC calls use gRPC
/// and the JSON-RPC SuiClient is not needed.
/// Returns true if the migration level is high enough that balance-related RPC calls
/// (including coin selection and balance retrieval) use gRPC.
///
/// Note: This does *not* guarantee that all RPC calls have been migrated to gRPC. Some
/// operations (for example, certain transaction queries or owned-object listing) may
/// still rely on the JSON-RPC `SuiClient`. Callers MUST NOT use this helper to decide
/// that a JSON-RPC client is unnecessary.
#[deprecated(
note = "is_fully_migrated only reflects that balance-related RPCs are on gRPC; \
do not use it to assume JSON-RPC is no longer required."
)]

Copilot uses AI. Check for mistakes.
pub fn is_fully_migrated(&self) -> bool {
*self >= GRPC_MIGRATION_LEVEL_GET_BALANCE
}
}

impl Default for GrpcMigrationLevel {
fn default() -> Self {
static VALUE: LazyLock<u32> = LazyLock::new(|| {
Expand Down
Loading