[rpc-alt] Use gRPC instead of JSON-RPC for Write API in sui-indexer-alt-jsonrpc#25860
[rpc-alt] Use gRPC instead of JSON-RPC for Write API in sui-indexer-alt-jsonrpc#25860
Conversation
Replace the JSON-RPC HTTP client delegation with gRPC calls for executeTransactionBlock and dryRunTransactionBlock. The Write module now uses FullnodeClient from sui-indexer-alt-reader, which communicates with the fullnode via TransactionExecutionService gRPC. - Add configurable read_mask parameter to FullnodeClient's execute_transaction and simulate_transaction methods - Add --fullnode-grpc-url flag (separate from existing --fullnode-rpc-url which remains for DelegationGovernance JSON-RPC delegation) - Implement proto-to-JSON-RPC response conversion for effects, events, balance changes, and object changes - Build dynamic FieldMask based on SuiTransactionBlockResponseOptions Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Move executeTransactionBlock and dryRunTransactionBlock tests from jsonrpc_fn_delegation_tests.rs into a new jsonrpc_write_api_tests.rs. The delegation test file now only tests governance APIs (getStakes, getStakesByIds, getValidatorsApy) and no longer needs a gRPC URL. The new write tests verify conversion correctness: sender identity in transaction input, exact balance change amounts for sender/recipient, effects status and gas costs, object change types, and consistency between dry run and execute results. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1c942d6 to
bab5c3e
Compare
| async fn test_dry_run_invalid_tx() { | ||
| telemetry_subscribers::init_for_testing(); | ||
| let cluster = WriteTestCluster::new().await.unwrap(); | ||
|
|
||
| let response = cluster | ||
| .execute_jsonrpc( | ||
| "sui_dryRunTransactionBlock", | ||
| json!({ "tx_bytes": "invalid_tx_bytes" }), | ||
| ) | ||
| .await | ||
| .unwrap(); |
There was a problem hiding this comment.
this is a nicer error than, "Invalid value was given to the function". makes me think though, are these technically breaking changes since the error string is different?
| // The transaction either fails at execution (effects with failure status) | ||
| // or is rejected by the validator during simulation (error response). | ||
| let has_failure_effects = response["result"]["effects"]["status"]["status"] == "failure"; | ||
| let has_error = response["error"].is_object(); | ||
| assert!( | ||
| has_failure_effects || has_error, | ||
| "expected failure effects or error response, got: {response}" | ||
| ); | ||
| } |
There was a problem hiding this comment.
is the response["error"] path just to handle transient errors? wouldn't we expect this to always have failure effects - it's a valid txn but gets aborted in the vm because not run by validator?
| .await | ||
| .unwrap(); | ||
|
|
||
| assert!(response["error"]["code"].is_number()); |
There was a problem hiding this comment.
any reason we aren't asserting on the exact error cod enumber?
| result.raw_effects = effects_bcs.value().to_vec(); | ||
| } | ||
|
|
||
| let effects: TransactionEffects = effects_bcs |
There was a problem hiding this comment.
i think we nly need to deserialize effects_bcs if show_effects or show_object_changes
| } | ||
|
|
||
| fn build_object_changes( | ||
| tx_data: &TransactionData, |
There was a problem hiding this comment.
cant we just pass the sender SuiAddress in instead of the full TransactionData?
| let mut objects: HashMap<(ObjectID, u64), Object> = HashMap::new(); | ||
| if let Some(object_set) = &executed_tx.objects { | ||
| for proto_obj in &object_set.objects { | ||
| if let Some(bcs) = &proto_obj.bcs { |
There was a problem hiding this comment.
when might an obj not have a bcs? when it's Wrapped or Deleted only? Given its a proto field could we be masking when the bcs should be present but isn't due to malformed data?
| o.version().value(), | ||
| ), | ||
|
|
||
| (ID::Created, _, None) => continue, |
There was a problem hiding this comment.
obj created and immediately wrapped?
| ), | ||
|
|
||
| (ID::Created, _, None) => continue, | ||
| (ID::None, None, _) => continue, |
There was a problem hiding this comment.
what scenario might result in this? If we have an unwrap and an output object it seems we'd skip it?
| (ID::None, _, Some((o, _))) if o.is_package() => continue, | ||
| (ID::Deleted, None, _) => continue, |
| Ok(SuiTransactionBlockEvents { data: sui_events }) | ||
| } | ||
|
|
||
| fn build_object_changes( |
There was a problem hiding this comment.
wait a moment, is it just me or does this look really similar to the object_Changes in transactions/response.rs? could we abstract and share the logic mapping the existence of an object in input, output, and the IDOperation?
There was a problem hiding this comment.
If there's only one real use case of execute_transaction and simulate_transaction that rely on field masks being set how they are, rather than using the current field mask values as defaults, can we make it a non-optional parameter and have these call-sites be explicit about what they expect?
Maybe there's a potential to consolidate the field masks that we use in GraphQL between execution and simulation as a result.
There was a problem hiding this comment.
Please take a look at the structure used for producing a transaction response for sui_getTransactionBlock (where we have a separate response module and each response option has its own associated helper function), and replicate that here.
Description
Replace the JSON-RPC HTTP client delegation with gRPC calls for executeTransactionBlock and dryRunTransactionBlock. The Write module now uses FullnodeClient from sui-indexer-alt-reader, which communicates with the fullnode via TransactionExecutionService gRPC.
Test plan
How did you test the new or updated feature?
Release notes
Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.
For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.