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

Add abstractions for converting between SCVal and native types. #500

Merged
merged 20 commits into from
Aug 14, 2023

Conversation

overcat
Copy link
Member

@overcat overcat commented Aug 4, 2023

Closes #487

Bocked by #498

This PR is mainly aimed at making it easier for users to build SCVal, such as constructing a uint256 type of SCVal. Users no longer need to worry about the internal details.

How users create and parse SCVal:

SCVal name0 = new ScvSymbol("overcat").toSCVal();
String name1 = ScvSymbol.fromSCVal(name0).getValue();

How users build a invoke function operation:

String contractId = "CBDMU2XMEUH6BCNIJEVMK5VCWK3TP75CPRDJF5ZY2BDU5MAIVCJ2GAPM";
String functionName = "hello";
List<Scv> params = Arrays.asList(new ScvSymbol("param_demo"), new ScvInt32(2134));
InvokeHostFunctionOperation operation = InvokeHostFunctionOperation.invokeContractFunctionOperationBuilder(contractId, functionName, params).build();

@overcat overcat marked this pull request as ready for review August 5, 2023 10:40
@overcat overcat changed the title [WIP] Add abstractions for converting between SCVal and native types. Add abstractions for converting between SCVal and native types. Aug 5, 2023
@sreuland
Copy link
Contributor

sreuland commented Aug 7, 2023

this is challenging to review, trying to focus on just the code that is related to conversions but the diff brings in all the changes from #498 also, can you rebase off https://github.com/overcat/java-stellar-sdk/tree/fix-xdr-unsigned, or change the merge target on this to https://github.com/overcat/java-stellar-sdk/tree/fix-xdr-unsigned ?

import org.stellar.sdk.xdr.SCValType;

/** Abstract class for all {@link Scv} types. */
public abstract class Scv {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you make all the ScvXyz.. classes in the org.stellar.sdk.scval package private?

that way these classes can't leak out to external clients, otherwise this could get overwhelming to have duplicate pojo models of the same thing.

Instead, this Scv class should be the only public class in org.stellar.sdk.scval and continue with the factory pattern you've already started here but add generics to handle the class type selector:

public static <T> T fromScVal(java.lang.class<T> nativeClass, SCVal scVal)
public static <T extends org.stellar.sdk.xdr.XdrElement> T toScValue(java.lang.class<T> scValClass, Object nativeObj)

both of these static factory methods use generics and the implementations use the class parameter as the selector for which of the package private ScvXyz.. classes to use for the actual conversion.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you, I will fix it later, let's review it again after we merge at #498.

Copy link
Member Author

@overcat overcat Aug 7, 2023

Choose a reason for hiding this comment

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

I think we still need to keep them as public because even if we add generic functions, users will still need to access these classes through public if they are required to pass in nativeClass.

Actually, most of the classes in the SDK are abstractions of the XDR layer, right? For example, the MemoId class corresponds to a lower-level XDR class. I believe this does not cause confusion.

So, I prefer to follow the design of Memo class, which means keeping the existing design unchanged but adding a series of functions for users to create SCVal through Scv, just like this: https://github.com/stellar/java-stellar-sdk/blob/master/src/main/java/org/stellar/sdk/Memo.java#L22-L87

demo: a0bde10

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think the presence of the wrapper pattern as used on other xdr entities means it's the best for fit ScVal model, because ScVal is more complex model with numerous sub-types which increases the code footprint alot rather than a one-to-one like Memo which has no type delegation.

In general, we do want to follow javascript api on general soroban interface usage, so there is some consistency across the sdk's, but it can diverge where things can be done more efficient based on the programming language capabilities. For now, the javascript sdk for soroban has implemented the factory method approach for ScVal conversions, as shown here for nativeToScVal() and scValToNative() .

I don't grasp the reason stated for needing public access to the ScvXyz.. conversion classes as part of nativeClass parameter usage. But, I think can reduce the code further by removing the ScvXyz.. classes and just inlining the conversion aspect of each into a switch handler in the static factory method. To walk through further in Java, the genericized factory methods for ScVal<->native java type conversions should be doable with:

public class Scv {
    public static <T> T fromScVal(java.lang.Class<T> nativeClass, SCVal scVal) {
       switch (scVal.getDiscriminant()) {
         case SCV_SYMBOL:
           return nativeClass.cast(new String(scVal.getSym().getSCSymbol().getBytes()));
       }

       throw new IllegalArgumentException("SCVal of type " + scVal.getDiscriminant().name() + " not supported");
    }

    public static SCVal toScValue(SCValType scValType, Object nativeObj) {
       switch (scValType) {
        case SCV_SYMBOL:
          if (!nativeObj.getClass().isAssignableFrom(String.class)) {
            throw new IllegalArgumentException("SCSymbol can only be converted from a string value.");
          }
          SCVal val =new SCVal();
          val.setSym(new SCSymbol(new XdrString(nativeObj.toString())));
          return val;
      }

      throw new IllegalArgumentException("SCVal of type " + scValType.name() + " not supported");

    }
}

example usage:

SCVal test = Scv.toScValue(SCValType.SCV_SYMBOL, "test");
String test2 = Scv.fromScVal(String.class, test);

Copy link
Member Author

@overcat overcat Aug 9, 2023

Choose a reason for hiding this comment

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

Your reasons do make sense, but I still believe that the original design is more user-friendly. If I were a user, I would prefer the original approach. Here is a demo of how users build a invoke function operation:

String contractId = "CBDMU2XMEUH6BCNIJEVMK5VCWK3TP75CPRDJF5ZY2BDU5MAIVCJ2GAPM";
String functionName = "hello";
List<Scv> params = Arrays.asList(new ScvSymbol("param_demo"), new ScvInt32(2134));
InvokeHostFunctionOperation operation = InvokeHostFunctionOperation.invokeContractFunctionOperationBuilder(contractId, functionName, params).build();

I don't grasp the reason stated for needing public access to the ScvXyz.. conversion classes as part of nativeClass parameter usage.

If the user needs to parse a Symbol in the example you provided, they would also need to know that symbols are represented by Strings and pass the String.class into the function(Scv.fromScVal(String.class, test)). However, with ScvSymbol, all the user needs to know is that it is a Symbol. The ScvSymbol.value already represents it as a String.

Hi @tamirms and @Shaptic, you have rich experience in maintaining SDKs, and I would like to hear your thoughts.

Copy link
Contributor

Choose a reason for hiding this comment

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

@overcat my preference is to have factory functions for each subtype of ScVal:

public class Scv {
    public static SCVal toSymbol(String symbol) {
        SCVal val =new SCVal();
        val.setSym(new SCSymbol(new XdrString(nativeObj.toString())));
        return val;
    }

public static String fromSymbol(SCVal scVal) {
       switch (scVal.getDiscriminant()) {
         case SCV_SYMBOL:
           return new String(scVal.getSym().getSCSymbol().getBytes());
       }

       throw new IllegalArgumentException("SCVal of type " + scVal.getDiscriminant().name() + " not supported");
    }
}

I like this approach because the mapping from ScVal type to native Java type is explicit and I think the developer experience is similar to what you proposed:

String contractId = "CBDMU2XMEUH6BCNIJEVMK5VCWK3TP75CPRDJF5ZY2BDU5MAIVCJ2GAPM";
String functionName = "hello";
List<SCVal> params = Arrays.asList(Scv.toSymbol("param_demo"), Scv.toInt32(2134));
InvokeHostFunctionOperation operation = InvokeHostFunctionOperation.invokeContractFunctionOperationBuilder(contractId, functionName, params).build();

Copy link
Contributor

Choose a reason for hiding this comment

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

the factory method per type sounds good, as the single genericized methods don't provide that type of static analysis.

Copy link
Member Author

Choose a reason for hiding this comment

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

This looks good, I have already submitted the refactored code.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, it's close, it's still defining ScvXyz classes as stateful pojo's which is extra code they don't need to be, to really drive the static factory design pattern, make each ScvXyz class be completely static, it's just a package private static helper used by Scv methods. As an example, here's how ScvDuration would look:

overcat-deprecation/java-stellar-sdk@scval-alias...sreuland:java-stellar-sdk:500_pr

Copy link
Member Author

Choose a reason for hiding this comment

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

@sreuland PTAL

Copy link
Contributor

@sreuland sreuland left a comment

Choose a reason for hiding this comment

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

I only reviewd the scval package as the rest of changes look like duplicate from #498

in regards to the scval conversions, I've made a suggestion comment for internalizing the converter classes to private and using static factory methods with generics, please respond with your thoughts on this before merging, thanks!

@overcat overcat marked this pull request as draft August 7, 2023 03:17
@overcat overcat marked this pull request as ready for review August 9, 2023 09:20
@overcat overcat requested a review from tamirms August 12, 2023 00:35
@tamirms tamirms merged commit c9b9f99 into lightsail-network:soroban Aug 14, 2023
@overcat overcat deleted the scval-alias branch August 21, 2023 11:51
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.

3 participants