Protocol 19 #416

sreuland · 2022-03-25T22:28:51Z

These are updates to the Java SDK to support Protocol 19.

This PR merges the feature branch that has already gone through prior reviews for specifics on CAP-21 and CAP-40 changes here:

https://github.com/stellar/java-stellar-sdk/pulls?q=is%3Apr+is%3Aclosed+label%3A%22Protocol+19%22

Closes #410
Closes #411
Closes #409
Closes #412

…er from CAP40 into StrKey encoding/decoding.

…ome pr feedback

…ntID for signed payload pojo to check discriminant on public key types

… instead of tx.timebounds

…loadSigner, pr feedback

Protocol 19 - CAP-40 Signed Payload Signer

…unds

…t coverage.

…s, more tests

…nbuild interface for consistency

…e functional approach instead

CAP 21, Transaction Preconditions and Account sequence fields

…e-built signatures onto a transaction

…yte expressions in test

payload signer signatures

#412: updated xdr files to latest from protocol 19 changes on core

Shaptic

some minor notes ⬇️

Shaptic · 2022-04-12T19:19:40Z

CHANGELOG.md

@@ -2,6 +2,26 @@

 As this project is pre 1.0, breaking changes may happen for minor version bumps. A breaking change will get clearly notified in this log.

+## 0.32.0 (Pending)


Can we add something about gradle? I don't know anything about Java build envs but maybe a note here about the changes would be helpful

could mention, but the gradle version upgrade is not an external facing change, there is no developer touchpoint on it, it ends up here as pre-downloaded bins of the new version that developers still run with same cli gradlew build|test, they don't need to do additional host system changes for the gradle upgrade.

Shaptic · 2022-04-12T19:21:01Z

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

- inner.getTimeBounds(),
- inner.getNetwork()
- );
+ this.mInner = new TransactionBuilder(inner.accountConverter, new Account(inner.getSourceAccount(), inner.getSequenceNumber() - 1), inner.getNetwork())


Is re-creating the account here problematic, since seqnum changes will not be reflected by the inner.getSourceAccount()?

shouldn't be, kept the interface of this FeeBumpTransaction.Builder the same, the parameters are one-way inbound, doesn't mutate state of those, the new local instance of account is created here to use the new TransactionBuilder interface, but the seqnum increment change that new builder applies on that account instance are ignored here, since that info was never propagated outward.

Shaptic · 2022-04-12T19:21:33Z

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

+ .setBaseFee((int)inner.getFee())
+ .addOperations(Arrays.asList(inner.getOperations()))
+ .addMemo(inner.getMemo())
+ .addPreconditions(new TransactionPreconditions.TransactionPreconditionsBuilder().timeBounds(inner.getTimeBounds()).build())


Can the final Transaction inner have other preconditions that need to be added here?

it's a V0 tx since it's in this block, so TimeBounds is the only precondition'able attribute available from that version of tx right?

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

Shaptic · 2022-04-12T19:27:26Z

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

@@ -157,11 +162,6 @@ public static ChallengeTransaction readChallengeTransaction(String challengeXdr,
 throw new InvalidSep10ChallengeException("The transaction sequence number should be zero.");
 }

- // verify that transaction has time bounds set, and that current time is between the minimum and maximum bounds.


looked redundant, the test will fail on L165 if it were null

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

Shaptic · 2022-04-12T19:30:53Z

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

+ }
+
+ mTimeoutSet = true;
+ if (timeout > 0) {


sanity check: there's no else case, so will there be no preconditions built if timeout is set to inf?

yes, good catch on the edge case, I've reviewed this and cleaned up and added more test coverage on setting timeouts.

Shaptic · 2022-04-12T19:32:02Z

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

+ preconditions.getV2().getTimeBounds().getMinTime().getTimePoint().getUint64(),
+ preconditions.getV2().getTimeBounds().getMaxTime().getTimePoint().getUint64()));
+ }
+ if (preconditions.getV2().getExtraSigners() != null && preconditions.getV2().getExtraSigners().length > 0) {


some/most of these can never be null, afaict

there is no enforcement of null check in the generated xdr java stub classes, an instance of the stub class, xdr.PreconditionsV2 could be constructed with the builder which doesn't require any of these attribs to be set, which results in nulls:

https://github.com/stellar/java-stellar-sdk/pull/416/files#diff-1c2812de0a0b421d83904a598bacc4351a3c80d9701f4d7260ca9cf1b4f7ad00R197

Co-authored-by: George <Shaptic@users.noreply.github.com>

Shaptic

🎉

Kirbyrawr · 2022-04-30T23:37:56Z

src/test/java/org/stellar/sdk/SetOptionsOperationTest.java

+ KeyPair source = KeyPair.fromSecretSeed("SC4CGETADVYTCR5HEAVZRB3DZQY5Y4J7RFNJTRA6ESMHIPEZUSTE2QDK");
+
+ @Test
+ public void testPaylodSignerKey() {


There is a typo here it should be testPayloadSignerKey

sreuland and others added 17 commits March 20, 2022 20:29

#410: updated xdr definitions for Protocol 19, add SignedPayload Sign…

90f3ad5

…er from CAP40 into StrKey encoding/decoding.

#410: use xdr data types for predicate time dimensions, incorporate s…

5bf518b

…ome pr feedback

#410: use xdr encoding/decoding for signed payload (de)ser, use Accou…

c44b1de

…ntID for signed payload pojo to check discriminant on public key types

#410: maintain backwards compliant Predicate

163b582

#411: upgrade gradle to 7.4

c239785

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

1717767

#410: regenerated all xdr from latest .x source on p19 branch

49b477d

Merge branch '410_cap40' into 411_cap21

c0314cf

#411: separated tx builder from tx, using tx preconditions.timebounds…

5cae54b

… instead of tx.timebounds

#410: consolidate validation of signed payload aspects into SignedPay…

c61a5d5

…loadSigner, pr feedback

Merge branch '410_cap40' into 411_cap21

a329560

Merge pull request #414 from sreuland/protocol_19

53befbf

Protocol 19 - CAP-40 Signed Payload Signer

Merge remote-tracking branch 'upstream/protocol_19' into 411_cap21

8e0ae07

#411: fixed gradle install to maven local task

8ad830e

411: fixed shadow jar name on publish

931e10c

#411: added docs to SequenceNumberStrategy

d56a3c1

#411: added new account sequence ledger/time fields from CAP-21

123f4fc

sreuland mentioned this pull request Mar 25, 2022

CAP 21, Transaction Preconditions and Account sequence fields #415

Merged

sreuland and others added 11 commits March 25, 2022 17:25

#411: renamed local precondition variable to 'm' prefix convention

4af0328

#411: removed mapping of infinite time bounds to same as null time bo…

25dcd81

…unds

#411: fixed xdr marshalling of preconditions and timebounds, more tes…

432ff5f

…t coverage.

#411: used getter method for accessing the timebounds

271ef3e

#411: consolidate TimeBounds usage a bit further through Precondition…

4d11a21

…s, more tests

#411: added more test coverage on v1/v2 precondition conversions

810dc04

#411: updated to latest protocol 19 xdr schemas

bf69ab8

#411: align the tx builder sequence number resolution closer to go tx…

5b85e78

…nbuild interface for consistency

#411: remove the now unused sequence number strategy classes, used th…

bbd9871

…e functional approach instead

#411: use original source account sequence number during tx build

4c8f5bf

Merge pull request #415 from sreuland/411_cap21

62ff27a

CAP 21, Transaction Preconditions and Account sequence fields

sreuland requested a review from Shaptic March 31, 2022 23:12

sreuland requested a review from tamirms March 31, 2022 23:13

sreuland and others added 5 commits April 7, 2022 23:21

#410: added payload signer signature generation and enabled adding pr…

4ed57f3

…e-built signatures onto a transaction

#410: removed unnecessary guard check on hint length and cleaned up b…

e6e1966

…yte expressions in test

Merge pull request #417 from sreuland/payloadsigner_decorated_sig

f61a675

payload signer signatures

#412: updated xdr files to latest from protocol 19 changes on core

32765bf

Merge pull request #418 from sreuland/update_xdr

ac90d53

#412: updated xdr files to latest from protocol 19 changes on core

Shaptic reviewed Apr 12, 2022

View reviewed changes

sreuland and others added 2 commits April 14, 2022 14:50

updated javadoc on key pair signing

9d0c3cf

Co-authored-by: George <Shaptic@users.noreply.github.com>

#412: fixed timeout configs on tx builder, more tests

7c0b210

sreuland requested a review from Shaptic April 14, 2022 23:54

#412: updated changelog to reflect final p19 changes

a6a36c5

Shaptic approved these changes Apr 18, 2022

View reviewed changes

sreuland merged commit fb6dbe9 into master Apr 18, 2022

Kirbyrawr reviewed Apr 30, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Protocol 19 #416

Protocol 19 #416

sreuland commented Mar 25, 2022 •

edited

Loading

Shaptic left a comment

Shaptic Apr 12, 2022

sreuland Apr 14, 2022

Shaptic Apr 12, 2022

sreuland Apr 14, 2022

Shaptic Apr 12, 2022

sreuland Apr 14, 2022

Shaptic Apr 12, 2022

sreuland Apr 14, 2022

Shaptic Apr 12, 2022

sreuland Apr 14, 2022

Shaptic Apr 12, 2022

sreuland Apr 18, 2022

Shaptic left a comment

Kirbyrawr Apr 30, 2022

		@@ -2,6 +2,26 @@

		As this project is pre 1.0, breaking changes may happen for minor version bumps. A breaking change will get clearly notified in this log.

		## 0.32.0 (Pending)

Protocol 19 #416

Protocol 19 #416

Conversation

sreuland commented Mar 25, 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

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

Shaptic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sreuland commented Mar 25, 2022 •

edited

Loading