-
Notifications
You must be signed in to change notification settings - Fork 160
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.
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
Added support for calling Soroban RPC server. #492
Added support for calling Soroban RPC server. #492
Conversation
make sure to change merge target on this to 'soroban' branch |
assume this depends on #481 to merge first as it will bring in all the new xdr definitions? |
21c7c89
to
c2b0a09
Compare
@@ -0,0 +1,14 @@ | |||
package org.stellar.sdk; | |||
|
|||
public class LedgerEntryNotFoundException extends Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe a style guideline to add, for any public class at minimum, add a javadoc for the class level, and would be nice to have javadoc on any public methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
private static final int SUBMIT_TRANSACTION_TIMEOUT = 60; // seconds | ||
private static final int CONNECT_TIMEOUT = 10; // seconds | ||
private final HttpUrl serverURI; | ||
private final OkHttpClient httpClient; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe this could be an opportunity to replace OkHttpClient because it's really bloated and it pulls in kotlin as a dependency even though it's not used here at all.
If you want to evaluate http clients a bit, the other alternatives are apache HttpClient or bump the java jdk level of project to 11 and then can use the jdk's built-in java.net.http.HttpClient
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, skip taking that on in here, we should just carve out changing the jdk level and updating library deps to a separate 'tech debt' ticket so it doesn't conflate the effort here, thanks!
cc @mollykarcher , @tamirms
this.httpClient = httpClient; | ||
} | ||
|
||
public TransactionBuilderAccount getAccount(String accountId) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would be helpful to javadoc the public methods, both for readability of source and also for client usage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
public GetLedgerEntriesResponse getLedgerEntries(Collection<LedgerKey> keys) throws IOException { | ||
List<String> xdrKeys = | ||
keys.stream().map(SorobanServer::ledgerKeyToXdrBase64).collect(Collectors.toList()); | ||
GetLedgerEntriesRequest params = new GetLedgerEntriesRequest(xdrKeys); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is an empty keys
list a valid request and/or should this check for empty list on xdrKeys
, to avoid sending to rpc in any case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
} | ||
|
||
public GetHealthResponse getHealth() throws IOException { | ||
return this.<Void, GetHealthResponse>sendRequest( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can remove the this
references, that scope is implied when any non-static method invokes another non-static method of class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but I think adding "this" is also very common. If you insist on removing it, I can do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's not common to use this
when the variable scope doesn't require it, one example of usage is when a method level variable shadows a class variable, but the usage here in this scope is not required and results in some code bloat.
import lombok.Getter; | ||
|
||
@Getter | ||
public class SorobanRpcErrorResponse extends RuntimeException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i expected this to be checked exception, forcing callers to handle it, what was the reason for coercing this to unchecked with extends RuntimeException
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, after more review, I don't think we want to model an rpc error response as an exception, rather it is conveyed as an attribute in the SorobanRpcResponse instead, thinking this SorobanRpcErrorResponse can be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After the discussion in #492 (comment), I think it would be better to directly return the result instead of SorobanRpcErrorResponse. What do you think?
Request request = new Request.Builder().url(this.serverURI).post(requestBody).build(); | ||
try (Response response = this.httpClient.newCall(request).execute()) { | ||
SorobanRpcResponse<R> sorobanRpcResponse = responseHandler.handleResponse(response); | ||
if (sorobanRpcResponse.getError() != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thinking about this, not sure that a successful http response with json-rpc error in the response payload should be elevated to an exception, instead, just return the sorobanRpcResponse
in all cases if you get it, as it has result and error attributes, that way clients only have to catch the IOException as a fatal type problem, otherwise they will inspect the response result/error for further application meaning. I commented similar on SorobanRpcErrorResponse
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, personally I prefer returning the entire SorobanRpcResponse
, but after reading the code of other libraries (including js-soroban-sdk), I found that they directly return RpcResponse.result.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a question, is it necessary to expose the "id" field in jsonrpc to users? In other words, when making a jsonrpc request via http, is the "id" field almost useless?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just in case, I'd wire it in for allowing it to be set by caller in sdk, but accepting null,etc so that a default hardcoded id can be applied, just so we don't get stuck.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If that's the case, I will still stick to the current design, which is to throw a SorobanRpcErrorResponse
exception if the response contains an error.
However, I will wrap the returned result, for example:
@Value
@AllArgsConstructor
class WrappedResponse<T> {
String id;
T result;
}
public WrappedResponse<TransactionBuilderAccount> getAccount(String accountId, String requestId)
Because interfaces like getAccount
and prepareTransaction
call RPC interfaces internally, but these interfaces do not directly return the data returned by Soroban-RPC to the user. This modification helps users obtain the ID.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After careful consideration, I suggest sticking with the current design. If there is a need to expose the id
in the future, we can add it without introducing any breaking changes..
src/main/java/org/stellar/sdk/requests/sorobanrpc/SorobanRpcRequest.java
Show resolved
Hide resolved
@AllArgsConstructor | ||
@Value | ||
public class SendTransactionResponse { | ||
@SerializedName("status") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can remove the @SerializedName annotations when it matches the java class member name, gson will by default serialize to that name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
Hi @sreuland, this PR is still in WIP status. I will let you know when it is ready for review. Thank you! |
This PR is almost complete, of course there are still a few documents and test cases waiting to be added, welcome to leave comments. |
Click to view an example of creating and submitting a Soroban transaction. Click me.package org.stellar.sdk;
import java.io.IOException;
import org.junit.Test;
import org.stellar.sdk.requests.sorobanrpc.SorobanRpcErrorResponse;
import org.stellar.sdk.responses.sorobanrpc.SendTransactionResponse;
import org.stellar.sdk.xdr.HostFunction;
import org.stellar.sdk.xdr.HostFunctionType;
import org.stellar.sdk.xdr.SCSymbol;
import org.stellar.sdk.xdr.SCVal;
import org.stellar.sdk.xdr.SCValType;
import org.stellar.sdk.xdr.SCVec;
import org.stellar.sdk.xdr.Uint32;
import org.stellar.sdk.xdr.XdrString;
public class DemoTest {
@Test
public void demo()
throws IOException,
SorobanRpcErrorResponse,
AccountNotFoundException,
PrepareTransactionException {
SorobanServer sorobanServer = new SorobanServer("http://100.83.15.43:8000/soroban/rpc");
String contractId = "CDCYWK73YTYFJZZSJ5V7EDFNHYBG4QN3VUNG2IGD27KJDDPNCZKBCBXK";
KeyPair txSubmitterKp =
KeyPair.fromSecretSeed("SAAPYAPTTRZMCUZFPG3G66V4ZMHTK4TWA6NS7U4F7Z3IMUD52EK4DDEV");
KeyPair opInvokerKp =
KeyPair.fromSecretSeed("SAEZSI6DY7AXJFIYA4PM6SIBNEYYXIEM2MSOTHFGKHDW32MBQ7KVO6EN");
TransactionBuilderAccount source = sorobanServer.getAccount(txSubmitterKp.getAccountId());
Transaction transaction =
new TransactionBuilder(AccountConverter.enableMuxed(), source, Network.STANDALONE)
.setBaseFee(50000)
.addPreconditions(
TransactionPreconditions.builder().timeBounds(new TimeBounds(0, 0)).build())
.addOperation(
InvokeHostFunctionOperation.builder()
.sourceAccount(opInvokerKp.getAccountId())
.hostFunction(
new HostFunction.Builder()
.discriminant(HostFunctionType.HOST_FUNCTION_TYPE_INVOKE_CONTRACT)
.invokeContract(
new SCVec(
new SCVal[] {
new Address(contractId).toSCVal(),
new SCVal.Builder()
.discriminant(SCValType.SCV_SYMBOL)
.sym(new SCSymbol(new XdrString("increment")))
.build(),
new Address(opInvokerKp.getAccountId()).toSCVal(),
new SCVal.Builder()
.discriminant(SCValType.SCV_U32)
.u32(new Uint32(10))
.build()
}))
.build())
.build())
.build();
Transaction newTx = sorobanServer.prepareTransaction(transaction);
newTx.sign(txSubmitterKp);
newTx.sign(opInvokerKp);
SendTransactionResponse resp = sorobanServer.sendTransaction(newTx);
System.out.println(resp);
}
} |
Hi @sreuland, this PR is ready. |
Closes #485, Closes #479, Closes #495