Skip to content
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

Merged
merged 16 commits into from
Aug 2, 2023

Conversation

overcat
Copy link
Member

@overcat overcat commented Jul 21, 2023

Closes #485, Closes #479, Closes #495

@sreuland
Copy link
Contributor

make sure to change merge target on this to 'soroban' branch

@sreuland
Copy link
Contributor

assume this depends on #481 to merge first as it will bring in all the new xdr definitions?

@overcat overcat changed the base branch from master to soroban July 21, 2023 22:42
@overcat
Copy link
Member Author

overcat commented Jul 22, 2023

assume this depends on #481 to merge first as it will bring in all the new xdr definitions?

Yes, let's merge #481 first.

@@ -0,0 +1,14 @@
package org.stellar.sdk;

public class LedgerEntryNotFoundException extends Exception {
Copy link
Contributor

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.

Copy link
Member Author

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;
Copy link
Contributor

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

Copy link
Contributor

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!

#496

cc @mollykarcher , @tamirms

this.httpClient = httpClient;
}

public TransactionBuilderAccount getAccount(String accountId)
Copy link
Contributor

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.

Copy link
Member Author

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);
Copy link
Contributor

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?

Copy link
Member Author

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(
Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Contributor

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 {
Copy link
Contributor

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?

Copy link
Contributor

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?

Copy link
Member Author

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) {
Copy link
Contributor

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

Copy link
Member Author

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.

Copy link
Member Author

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?

Copy link
Contributor

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.

Copy link
Member Author

@overcat overcat Jul 28, 2023

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.

Copy link
Member Author

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..

@AllArgsConstructor
@Value
public class SendTransactionResponse {
@SerializedName("status")
Copy link
Contributor

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@overcat
Copy link
Member Author

overcat commented Jul 27, 2023

Hi @sreuland, this PR is still in WIP status. I will let you know when it is ready for review. Thank you!

@overcat
Copy link
Member Author

overcat commented Jul 28, 2023

This PR is almost complete, of course there are still a few documents and test cases waiting to be added, welcome to leave comments.

@overcat
Copy link
Member Author

overcat commented Jul 28, 2023

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);
  }
}

@overcat overcat changed the title [WIP] Added support for calling Soroban RPC server. Added support for calling Soroban RPC server. Jul 29, 2023
@overcat overcat marked this pull request as ready for review July 29, 2023 15:50
@overcat
Copy link
Member Author

overcat commented Jul 31, 2023

Hi @sreuland, this PR is ready.

@tamirms tamirms merged commit 25dc8a8 into lightsail-network:soroban Aug 2, 2023
@overcat overcat deleted the soroban-server branch August 11, 2023 08:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for Soroban RPC server. java-stellar-sdk: add support for fees
3 participants