[rpc-alt] add write apis #21153
[rpc-alt] add write apis #21153
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
2 Skipped Deployments
|
emmazzz
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
| #[tokio::test] | ||
| async fn test_execution_and_dry_run() { | ||
| // Set up a test cluster so we have some accounts and a fullnode RPC URL to connect to. | ||
| let test_cluster = TestClusterBuilder::new() |
There was a problem hiding this comment.
I thought about adding support for txn execution via rpc in transactional test runner but I figured this way is easier.
emmazzz
force-pushed
the
emma/rpc-alt-write
branch
from
4c8ded8 to
125b3a8
Compare
emmazzz
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
emmazzz
force-pushed
the
emma/rpc-alt-write
branch
from
125b3a8 to
5f32608
Compare
emmazzz
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
There was a problem hiding this comment.
Could you move this next to the other E2E tests? (see #21161 for what I mean).
| @@ -76,6 +84,7 @@ impl RpcService { | |||
| let RpcArgs { | |||
| rpc_listen_address, | |||
| max_rpc_connections, | |||
| .. | |||
There was a problem hiding this comment.
This is a sign that fullnode_rpc_url doesn't belong on this struct -- imagine that this module is similar to Indexer and IndexerArgs (part of an RPC service framework).
Can you create a separate clap Args struct for the WriteArgs, in the write module, instead? Then you can flatten that struct into the rpc sub-command's list of fields. That way you can keep RpcArgs as just the flags passed to initialise the core of the RPC service, but still accept the fullnode URL as a command-line argument.
| use super::rpc_module::RpcModule; | ||
|
|
||
| #[open_rpc(namespace = "sui", tag = "Write API")] | ||
| #[rpc(server, client, namespace = "sui")] |
There was a problem hiding this comment.
Generating the client based on this trait is neat
| /// 1. WaitForEffectsCert: waits for TransactionEffectsCert and then return to client. | ||
| /// This mode is a proxy for transaction finality. | ||
| /// 2. WaitForLocalExecution: waits for TransactionEffectsCert and make sure the node | ||
| /// executed the transaction locally before returning the client. The local execution | ||
| /// makes sure this node is aware of this transaction when client fires subsequent queries. | ||
| /// However if the node fails to execute the transaction locally in a timely manner, | ||
| /// a bool type in the response is set to false to indicated the case. |
There was a problem hiding this comment.
What's the latest on deprecating WaitForLocalExecution? Should we be supporting different request types? Does WaitForLocalExecution still have meaning for us, given "local" means the fullnode, and not this RPC service? We can no longer guarantee that the RPC will return a response that is consistent with the effects of this transaction, after executeTransactionBlock(..., WaitForLocalExecution) returns.
| .0 | ||
| .execute_transaction_block(tx_bytes, signatures, opts, request_type) | ||
| .await | ||
| .map_err(internal_error_object)?) |
There was a problem hiding this comment.
IIUC, this means that any error, including user errors, transient errors, etc from the fullnode will turn into internal errors here. That seems wrong -- we should be passing the errors on transparently.
| @@ -185,6 +194,7 @@ impl Default for RpcArgs { | |||
| Self { | |||
| rpc_listen_address: "0.0.0.0:6000".parse().unwrap(), | |||
| max_rpc_connections: 100, | |||
| fullnode_rpc_url: "http://localhost:9000".to_string(), | |||
There was a problem hiding this comment.
I was thinking about this default -- what are your thoughts on making this flag optional, and only adding the write module if it exists? It gets around the tricky issue of coming up with a default URL.
|
|
||
| /// The URL of the fullnode RPC we connect to for executing transactions. | ||
| #[clap(long, default_value_t = Self::default().fullnode_rpc_url)] | ||
| pub fullnode_rpc_url: String, |
There was a problem hiding this comment.
Let's use Url for this, so we get some input validation. The type implements AsRef<str> so it can be passed to HttpClientBuilder::build.
| // Set up the rpc server that we want to test. | ||
| let (_rpc_server_handle, rpc_url) = set_up_rpc_server(fullnode_rpc_url, cancel.clone()).await; | ||
|
|
||
| // Coonstruct a transaction to execute. |
There was a problem hiding this comment.
| // Coonstruct a transaction to execute. | |
| // Construct a transaction to execute. |
| const DEFAULT_GAS_AMOUNT: u64 = 1_000_000_000; | ||
|
|
||
| #[tokio::test] | ||
| async fn test_execution_and_dry_run() { |
There was a problem hiding this comment.
Let's add some tests for various error cases, and make sure the error codes get reported faithfully.
Description
This PR adds transaction execution and dry run APIs to indexer alt jsonrpc, which is basically a wrapper of the fullnode rpc.
Test plan
Added a test.
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.