txnbuild: Add support for new CAP-21 preconditions.

[Shaptic]

PR Checklist

PR Structure

• This PR has reasonably narrow scope (if not, break it down into smaller PRs).
• This PR avoids mixing refactoring changes with feature changes (split into two PRs
  otherwise).
• This PR's title starts with name of package that is most changed in the PR, ex.
  services/friendbot, or all or doc if the changes are broad or impact many
  packages.

Thoroughness

• This PR adds tests for the most critical parts of the new functionality or fixes.
• I've updated any docs (developer docs, .md
  files, etc... affected by this change). Take a look in the docs folder for a given service,
  like this one.

Release planning

• I've updated the relevant CHANGELOG (here for Horizon) if
  needed with deprecations, added features, breaking changes, and DB schema changes.
• I've decided if this PR requires a new major/minor version according to
  semver, or if it's mainly a patch change. The PR is targeted at the next
  release branch if it's not a patch change.

What

Adds support for new CAP-21 preconditions. This starts with a very simple API change:

@@ -212,7 +212,7 @@ type Transaction struct {
     sourceAccount SimpleAccount
     operations    []Operation
     memo          Memo
-    timebounds    Timebounds
+    preconditions Preconditions
 }

@@ -799,23 +799,23 @@ type TransactionParams struct {
     BaseFee       int64
     Memo          Memo
-    Timebounds    Timebounds
+    Preconditions Preconditions
 }

and evolves into all of the changes necessary to make that happen: the new Preconditions structure (creation, validation, XDR encoding, etc.) and coalescing between V1 conditions (timebounds only) and V2 conditions (CAP-21, incl. timebounds).

Why

Closes #4282.

[sreuland]

is this validation rule worth maintaining on client side, or let it get submitted and have core return txsub errors since it is source of truth on this aspect?

[Shaptic]

this will already blow up client-side because the XDR won't encode (not stellar core validation):

SignerKey extraSigners<2>;

this was just being a little friendlier about it, since it's not fixed to be 2 w/in Preconditions. no strong opinion on keeping it, though.

[sreuland]

interesting, on the java sdk side, I checked the generated java from xdrgen, PreconditionsV2.java, it doesn't capture the SignerKey extraSigners<2> size limit like it is on go with xdrmaxsize, I may need to hardcode that as a constant or just document that it's by default 2 and submission will be final validation

[Shaptic]

That's interesting - seems like a limitation of the generator? 🤔 definitely think it's worth a better error message from the SDK if possible

[leighmcculloch]

👏🏻. A few comments inline.

[Shaptic]

@leighmcculloch @sreuland I made the breaking change to move TransactionParams.Timebounds -> TransactionParams.Preconditions.Timebounds in 468e7de. Like I mentioned, it's simple but broad, so let me know what you guys think.

[Shaptic]

I've also forked off an alternative, non-breaking method. This one keeps Preconditions.timebounds private, but makes it a non-pointer. The branch diff is here: horizon-protocol-19...Shaptic:txnbuild-tx-conds-alternative, but the main change I think we all care about is here (no more coalescing or pointer voodoo).

Let me know which approach y'all like better 🕵️‍♂️ I personally think the latter (non-breaking) is not bad and an improvement over the issues everyone raised above. It aligns closely to (3) from #4303 (comment).

[leighmcculloch]

🎉 I like the breaking change moving TimeBounds. While it is a breaking change, I think the alternatives all come with significant tradeoffs.

I left some requests (❗), and also some suggestions (💡).

[leighmcculloch]

💡 The way this set of tests work by cloning things is particularly complicated. How about just writing out each input verbosely? It would be much easier to understand and maintain.

The inspiration for why I'm suggesting this is that DRY tests are often harder to maintain:

• https://thoughtbot.com/blog/the-case-for-wet-tests
• https://evhaus.medium.com/using-dry-wet-damp-code-6ab3c8245bb2

[Shaptic]

Obviously your perspective has a lot of weight, so I'm trying to maximize my own edification here by offering counterarguments. I feel like I should write my own blog post in response 😂 so a couple of thoughts follow.

---

How is it easier to understand? More than once I've opened a test file for a complex piece of code, and seen what appeared to be a lot of duplication across tests. It begged a really important question, though: are they actually the same? If you're reading page after page of code for related tests, you need to read each test really carefully to identify where it differs. If the "setup" is kept in one place, the tests can focus exclusively on their differences. That makes it easier to understand, not harder, imo.

Small case study: Take a look at the various operation tests in txnbuild/transaction_test.go. Different test groups have different authors, so irrelevant deltas get introduced. Why do the sequence numbers differ across the tests? Is it just an author change, or does it impact the test somehow? Some of them use NewTransaction, while others use newSignedTransaction. Does that make a difference, and why? Should I pay attention to each test's TransactionParams, or are they just copy-pasted across all test cases?

All of these questions are overhead to understanding the tests. If there was a single method that prepared the common parts of the test case, each one could focus on the differences between them. That's the goal of the "modifier" design I threw in: it's very clear (though I will admit I'm biased as the author and reader) that the only difference between each subtest is the fields set on the two preconditions.

---

Then there's the aspect of maintainability. How does that get easier? Within this PR itself, we had a breaking change re: timebounds that resulted in many lines changing. If there was a common place for the tests to prepare things (like for transaction_test.go as I mentioned above), there'd be a single delta of Timebounds: NewInfiniteTimeout() -> Preconditions: Preconditions{Timebounds: NewInfiniteTimeout()}, because that line is common to all of the tests. Now, as a reviewer I'd have to look at each line that changed and make sure it was updated correctly and no copy-paste or search&replace error snuck in.

Adding tests becomes a chore, too. Like I mentioned earlier (#4303 (comment)), there are a lot of ways to combine preconditions, now. We want all of them to be possible, and of course we can't exhaust them all (or can rely on bug reports for ones that somehow aren't possible), but it'd be nice to get the basics covered. Adding a new combination is a few LoC. Furthermore, extending the tests (like you suggested above to add a round-trip, #4303 (comment)) is so much easier when they're all testing common functionality. That extension took ~7 LoC in cf15638. If the tests were split out, that'd be N copy-pastes and 7*N lines of testing code that all do the same thing.

---

I guess I just... don't really get it? Yes, there's some degree of "cleverness" when reading the current code for the first time, but it comes at the (to me, huge) benefit of very clearly isolating the differences between each subtest case and allowing simple extensibility to add new combinations of preconditions.

As a final, disjoint thought: Something that makes even more sense than my current list of subtests is just "fuzzing" the fields on Precondition to ensure they all work. That's the ultimate goal here, anyway: answering the question of "Can all valid preconditions be created?"

[leighmcculloch]

It's a tradeoff for sure. Even if DRY tests look good on day one, imx they are harder to maintain over time, because the way they are structured are making assumptions about the tests we'll have to write in the future or the ways we'll need to modify these tests, requiring test refactors, etc. My comment was a suggestion, not a request.

If you are going to keep the cloning, might I suggest replacing it with a helper that creates the original structure. This way you don't need to write cloning code. (Cloning code is unavoidably brittle code.)

[Shaptic]

That's a good point about test evolution. Cross that bridge when we get to it, then? 😆 Either way, just want to reiterate that I really appreciate the feedback and suggestions, as it makes me challenge my own assumptions and get a better understanding of the trade-offs I may be making w/o realizing.

You're on the money about cloning: I hated that aspect of it. I opted to make "test fixtures" with a helper function in 84c882e8c9bd52ef93954392619d2e686f503882; I think it's much less brittle now overall!

[Shaptic]

Last round of feedback, I hope, @leighmcculloch and @stellar/horizon-committers? 🙏

To make reviews easier, here are the files with relevant (i.e. not just adapting to the breaking TransactionParams delta) changes:

• new files: txnbuild/ledgerbounds.go, txnbuild/preconditions.go, txnbuild/preconditions_test.go
• most of txnbuild/transaction.go
• the top of txnbuild/transaction_test.go
• txnbuild/timebounds.go
• xdr/transaction_envelope.go
• txnbuild/CHANGELOG.md

[leighmcculloch]

Typo: Ledgerbounds => LedgerBounds

[Shaptic]

Consistency with txnbuild.Timebounds? 🤔

[leighmcculloch]

You know, we could safely fix the Timebounds typo as part of this protocol release. By doing these things:

• We're already moving the Timebounds field, so simply name the new field TimeBounds.
• We can rename txnbuild.Timebounds to txnbuild.TimeBounds.
• We can use a type alias to specify that txnbuild.Timebounds is the same type as txnbuild.TimeBounds. Just add this:

  type Timebounds = TimeBounds

  The = will make it so that whenever the compiler sees Timebounds it'll pretend it saw TimeBounds. All the functions will be automatically lifted. No type casting required because they are one type, not two. Note this is different to type definitions that do not have the equals.

[Shaptic]

I'm good with that! We still have inconsistencies (see #4297 (comment)), and we can't rename the constructors w/o more breaking changes (e.g. NewTimebounds), but it's still progress 👷‍♂️. Both renames done in latest push 👍

[sreuland]

what is the benefit for using this abstraction model rather than using xdr.PreconditionsV2 direct? This and TimeBounds and LedgerBounds appear to be 1-1 proxy of their xdr counterparts?

[Shaptic]

Hm, didn't see this comment for some reason. They're 1-to-1 now, but the txnbuild package in general avoids exposing XDR to SDK consumers.

[sreuland]

ok, see that de-coupling at top level, sounds good, there are nested references from here into xdr types such as SignerKey and Duration, and was looking at it from bottom up, thanks!