Update XDR for Protocol 19, add encoding support for CAP-40 SignedPayload Signer #413

sreuland · 2022-03-21T03:41:15Z

Update the Java SDK to have the new XDR schema for Protocol 19

this change focuses on supporting CAP-40 , namely the new Signed Payload Signer as new StrKey.

Update the StrKey encoding/decoding to include the new Signed Payload Signer, add more tests where signed payload signer could appear such as SetOptionsOperation and AccountEntry.

Closes #412
Closes #410

can ignore all changed files in src/main/java/org/stellar/sdk/xdr, they are generated code from xdrgen tool against the latest xdr schema files.

This is first half of Protocol 19 support, a follow-on PR will be coming for remaining Protocol 19 support of CAP-21 which introduces new tx pre-conditions on of which is extraSigners which this new Signed Payload Signer can be used.

…ignedPayload Signer from CAP40 into StrKey encoding/decoding.

Shaptic

Definitely coming along, feedback is below!

src/main/java/org/stellar/sdk/Predicate.java

src/main/java/org/stellar/sdk/SignedPayloadSigner.java

src/main/java/org/stellar/sdk/StrKey.java

src/main/java/org/stellar/sdk/Transaction.java

Shaptic · 2022-03-21T18:16:18Z

src/main/java/org/stellar/sdk/Transaction.java

+    if (envelope.getTx().getCond().getDiscriminant().equals(PreconditionType.PRECOND_TIME)) {
+      mTimeBounds = TimeBounds.fromXdr(envelope.getTx().getCond().getTimeBounds());
+    }


necessary but not sufficient: there can also be timebounds on the PRECOND_V2 type. maybe a helper is in order for a general way to retrieve timebounds given a tx?

yes, I'll use that suggestion, will be in the next follow-on PR that adds the CAP21 support on those new conditions, I included all the xdr schema changes up front in this pr, but only did the cap40 changes above first, this part ideally was just trying to refactor to maintain present compile/test until then.

a way to do it here actually would be to say instead .notEquals(PreconditionType.PRECOND_NONE), since both other forms have a timebound condition 🤔

src/main/java/org/stellar/sdk/SignedPayloadSigner.java

…ns, incorporate some pr feedback

…de)ser, use AccountID for signed payload pojo to check discriminant on public key types

src/main/java/org/stellar/sdk/SignedPayloadSigner.java

Shaptic · 2022-03-22T23:00:34Z

src/main/java/org/stellar/sdk/Signer.java

 import static com.google.common.base.Preconditions.checkNotNull;

 /**
 * Signer is a helper class that creates {@link org.stellar.sdk.xdr.SignerKey} objects.
 */
 public class Signer {
+    public static final int SIGNED_PAYLOAD_MAX_PAYLOAD_LENGTH = 64;


Ah yeah, I think this belongs above in SignedPayloadSigner

ok, I had located validation here in the Signer.signedPayload(SignedPayloadSigner) factory method which constructs the xdr SignerKey for payload signer from the payload. The intent on SignedPayloadSigner was to be data transfer object, no logic, to your point, it could take on that role also of validation, but thought was keeping that logic closer to when it's used in SignerKey, let me know if you want to discuss more, thanks!

You do have validation in two places (signer key and strkey), and imo you'd never want a SignedPayloadSigner that wasn't valid, but your separation of concerns also makes sense. No strong opinion here:+1:

Shaptic · 2022-03-22T23:01:38Z

src/main/java/org/stellar/sdk/StrKey.java

+            char[] encoded = encodeCheck(VersionByte.SIGNED_PAYLOAD, record.toByteArray());
+            return String.valueOf(encoded);
+        } catch (Exception ex) {
+            throw new FormatException(ex.getMessage());


Why rethrow when you can just let it bubble up?

to muffle the checked exceptions which otherwise would require adding on a throws declaration on the method for each one, which then forces the callers to specifically handle it, it's mostly for caller convenience as there is no significant app-level meaning to a checked failure that the client could catch and do something else instead, rather they should just let it go up also.

makes sense, java-isms.. 👍

Shaptic · 2022-03-22T23:02:55Z

src/main/java/org/stellar/sdk/Transaction.java

+    if (envelope.getTx().getCond().getDiscriminant().equals(PreconditionType.PRECOND_TIME)) {
+      mTimeBounds = TimeBounds.fromXdr(envelope.getTx().getCond().getTimeBounds());
+    }


a way to do it here actually would be to say instead .notEquals(PreconditionType.PRECOND_NONE), since both other forms have a timebound condition 🤔

src/main/java/org/stellar/sdk/xdr/Asset.java

src/main/java/org/stellar/sdk/xdr/ClaimableBalanceID.java

…19 branch

Shaptic

1 small note of clarification around encoding, otherwise LGTM

Shaptic · 2022-03-23T21:30:19Z

src/main/java/org/stellar/sdk/Signer.java

 import static com.google.common.base.Preconditions.checkNotNull;

 /**
 * Signer is a helper class that creates {@link org.stellar.sdk.xdr.SignerKey} objects.
 */
 public class Signer {
+    public static final int SIGNED_PAYLOAD_MAX_PAYLOAD_LENGTH = 64;


You do have validation in two places (signer key and strkey), and imo you'd never want a SignedPayloadSigner that wasn't valid, but your separation of concerns also makes sense. No strong opinion here:+1:

Shaptic · 2022-03-24T16:17:17Z

src/main/java/org/stellar/sdk/StrKey.java

+            char[] encoded = encodeCheck(VersionByte.SIGNED_PAYLOAD, record.toByteArray());
+            return String.valueOf(encoded);
+        } catch (Exception ex) {
+            throw new FormatException(ex.getMessage());


makes sense, java-isms.. 👍

Shaptic · 2022-03-24T16:17:53Z

src/main/java/org/stellar/sdk/StrKey.java

            // Why not use base32Encoding.encode here?
            // We don't want secret seed to be stored as String in memory because of security reasons. It's impossible
            // to erase it from memory when we want it to be erased (ASAP).
            CharArrayWriter charArrayWriter = new CharArrayWriter(unencoded.length);
-            OutputStream charOutputStream = StrKey.base32Encoding.encodingStream(charArrayWriter);
+            OutputStream charOutputStream = base32Encoding.encodingStream(charArrayWriter);


Errm, note the comment above this line? I think there's a reason we use StrKey? Maybe @tamirms knows more on this (or git blame it)

this comment was referring to the usage of .encode() vs .encodingStream(), the change here was just source tidy-up, StrKey.base32Encoding was redundant, as within StrKey class scope, it references the same static member declared here at base32Encoding

…ts into SignedPayloadSigner, pr feedback

sreuland · 2022-03-24T21:42:37Z

You do have validation in two places (signer key and strkey), and imo you'd never want a SignedPayloadSigner that wasn't valid

@Shaptic , good point, since it spread to both, best to encapsulate once in the shared data object, updated c61a5d531371

lightsail-network#410: updated xdr definitions for Protocol 19, add S…

90f3ad5

…ignedPayload Signer from CAP40 into StrKey encoding/decoding.

sreuland requested a review from a team March 21, 2022 16:06

sreuland mentioned this pull request Mar 21, 2022

Protocol 19 SDK Support #409

Closed

4 tasks

Shaptic reviewed Mar 21, 2022

View reviewed changes

sreuland added 3 commits March 21, 2022 15:20

lightsail-network#410: use xdr data types for predicate time dimensio…

5bf518b

…ns, incorporate some pr feedback

lightsail-network#410: use xdr encoding/decoding for signed payload (…

c44b1de

…de)ser, use AccountID for signed payload pojo to check discriminant on public key types

lightsail-network#410: maintain backwards compliant Predicate

163b582

sreuland requested review from leighmcculloch and Shaptic March 22, 2022 18:40

Shaptic reviewed Mar 22, 2022

View reviewed changes

sreuland added 2 commits March 23, 2022 10:42

lightsail-network#410: renamed parameter to 'signer' per feedback on pr

1717767

lightsail-network#410: regenerated all xdr from latest .x source on p…

49b477d

…19 branch

sreuland requested a review from Shaptic March 23, 2022 18:28

Shaptic approved these changes Mar 24, 2022

View reviewed changes

lightsail-network#410: consolidate validation of signed payload aspec…

c61a5d5

…ts into SignedPayloadSigner, pr feedback

sreuland closed this Mar 24, 2022

sreuland deleted the 410_cap40 branch March 24, 2022 22:18

sreuland mentioned this pull request Mar 24, 2022

Protocol 19 - CAP-40 Signed Payload Signer #414

Merged

sreuland added the Protocol 19 label Apr 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update XDR for Protocol 19, add encoding support for CAP-40 SignedPayload Signer #413

Update XDR for Protocol 19, add encoding support for CAP-40 SignedPayload Signer #413

sreuland commented Mar 21, 2022 •

edited

Loading

Shaptic left a comment

Shaptic Mar 21, 2022

sreuland Mar 21, 2022

Shaptic Mar 22, 2022

Shaptic Mar 22, 2022

sreuland Mar 23, 2022

Shaptic Mar 23, 2022

Shaptic Mar 22, 2022

sreuland Mar 23, 2022 •

edited

Loading

Shaptic Mar 24, 2022

Shaptic Mar 22, 2022

Shaptic left a comment

Shaptic Mar 23, 2022

Shaptic Mar 24, 2022

Shaptic Mar 24, 2022

sreuland Mar 24, 2022

sreuland commented Mar 24, 2022

Update XDR for Protocol 19, add encoding support for CAP-40 SignedPayload Signer #413

Update XDR for Protocol 19, add encoding support for CAP-40 SignedPayload Signer #413

Conversation

sreuland commented Mar 21, 2022 • edited Loading

Shaptic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sreuland Mar 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shaptic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sreuland commented Mar 24, 2022

sreuland commented Mar 21, 2022 •

edited

Loading

sreuland Mar 23, 2022 •

edited

Loading