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

Horizon: Protocol 19 Integration Tests #4305

Closed
7 tasks done
Tracked by #4261
Shaptic opened this issue Mar 25, 2022 · 7 comments
Closed
7 tasks done
Tracked by #4261

Horizon: Protocol 19 Integration Tests #4305

Shaptic opened this issue Mar 25, 2022 · 7 comments
Assignees
Labels
Protocol 19 Support cap-21, cap-40 and sep-23 in Horizon & SDKs

Comments

@Shaptic
Copy link
Contributor

Shaptic commented Mar 25, 2022

Breakdown and Rollout: Horizon & Protocol 19. See #4261 for the master issue.


Now that Protocol 19 feature work is mostly complete in the horizon-protocol-19 branch and the Go SDK is up-to-date, we can turn our focus to stability. We need end-to-end integration tests for the new features:

@Shaptic Shaptic added the Protocol 19 Support cap-21, cap-40 and sep-23 in Horizon & SDKs label Mar 25, 2022
@jcx120
Copy link

jcx120 commented Mar 25, 2022

Are there tests we should include for regressing re-ingestion and regular ingestion to keep up with ledger?

@Shaptic
Copy link
Contributor Author

Shaptic commented Mar 28, 2022

@jcx120 added an item re: state verifier. I think we get regression testing "automatically" by running our test suite without an actual Protocol 19 build of Stellar Core (aka what's been happening until #4306 gets merged), but moving forward yes, we need to ensure each new feature has the relevant ingestion testing (I think it did in their individual PRs).

@2opremio 2opremio self-assigned this Mar 30, 2022
@sreuland sreuland self-assigned this Apr 1, 2022
2opremio pushed a commit to 2opremio/go-2 that referenced this issue Apr 4, 2022
sreuland added a commit to 2opremio/go-2 that referenced this issue Apr 4, 2022
sreuland added a commit to sreuland/go that referenced this issue Apr 5, 2022
sreuland added a commit to sreuland/go that referenced this issue Apr 5, 2022
sreuland added a commit to sreuland/go that referenced this issue Apr 6, 2022
sreuland added a commit to sreuland/go that referenced this issue Apr 6, 2022
sreuland added a commit that referenced this issue Apr 11, 2022
Added additional round trip verification by re-querying preconditions on tx after submission, per pr feedback
@2opremio
Copy link
Contributor

2opremio commented Apr 12, 2022

Add state verifier tests for all of the above

@Shaptic how do you suggest we do this? Changing TestStateVerifier to submit transactions with preconditions? Or are you just referring to the new Account fields?

Because I think the latter is already implicitly tested.

@2opremio
Copy link
Contributor

Test new transaction preconditions

I think this is almost done, just a few fields left. @sreuland are you on top of it? (since you where already adding tests for other preconditions)

@sreuland
Copy link
Contributor

I think this is almost done, just a few fields left. @sreuland are you on top of it? (since you where already adding tests for other preconditions)

There are couple tests remaining, started working on coverage for LedgerBounds, MinSequenceNumberAge, MinSequenceLedgerNumberGap in Preconditions.

Also, noticed the xdr files changed recently on core p19 branch such as Stellar-transaction.x. Do we need to sync up to those?

@sreuland
Copy link
Contributor

@2opremio , PR #4332 is up for new integration test coverage on those remaining transaction preconditions.

@Shaptic
Copy link
Contributor Author

Shaptic commented Apr 13, 2022

@2opremio You're right that account extensions were supposed to have been tested! And preconditions aren't part of state, so we don't need to test those.

Unfortunately, the account extensions actually weren't tested after #4304 because this line:

{randxdr.FieldEquals("created.data.account.ext.v1.ext.v2.ext.v"), randxdr.SetU32(0)},

kept the extension disabled and I didn't realize that during the review 🤦

This is resolved by #4337.

@Shaptic Shaptic closed this as completed Apr 14, 2022
Shaptic added a commit that referenced this issue Apr 19, 2022
* all: Add Protocol 19 XDR and update StrKey to support Signed Payloads (#4279)
* services/horizon: Support new account fields for protocol-19. (#4294)
* xdr, keypair: Add helpers to create CAP-40 decorated signatures (#4302)
* services/horizon: Update txsub queue to account for new CAP-21 preconditions (#4301)
* txnbuild: Add support for new CAP-21 preconditions. (#4303)

Adds support for new CAP-21 preconditions with a simple, but breaking API
change:

TransactionParams.Timebounds -> TransactionParams.Preconditions.TimeBounds

You can now pass a lot more preconditions to a Transaction (including
timebounds), and these are managed by the new Preconditions object. All of the
XDR abstractions are hidden away behind this object.

* services/horizon: Support new CAP-21 transaction conditions (#4297)
* txnbuild: Complete rename, avoid using XDR types in `TransactionParams`. (#4307)
* all: Update Protocol 19 XDR to the latest (#4308)
* horizon: Set up protocol 19 integration tests infrastructure (#4312)
* horizon: Test new protocol 19 account fields (#4322)
* horizon: Add integration test for extra signers using tx preconditions (#4305)
* horizon: Add transaction submission test for Protocol 19 (#4327)
* services/horizon: Use `bigint` over `timestamp` to accommodate large years (#4337)

This is necessary because PostgreSQL puts "reasonable" limitations
on how dates can be represented. Because the state verifier test
will generate random int64s for the V3 account extension field
introduced in CAP-21, this will blow up on inserting into the DB
with errors like:

    pq: date/time field value out of range: "120911623444-02-01 21:35:20Z"

because PostgreSQL only allows years up to 5874897
(https://stackoverflow.com/a/36446977).

Technically, since this field is not user-controlled and comes
directly from Stellar Core, we would not see this in practice.

However, it's a more durable fix to stop the extra layer of
"interpretation" of this value and just treat it like it is:
a 64-bit integer that happens to represent a UNIX timestamp.

* services/horizon: Change `min_account_sequence_age` column from `bigint` to string (#4339)
* Add changelog details and prep v10.0.0 SDK release

Co-authored-by: Alfonso Acosta <alfonso@stellar.org>
Co-authored-by: erika-sdf <92893480+erika-sdf@users.noreply.github.com>
Co-authored-by: Paul Bellamy <paul@stellar.org>
Co-authored-by: Shawn Reuland <shawn@stellar.org>
Co-authored-by: shawn <sreuland@users.noreply.github.com>
Co-authored-by: George <Shaptic@users.noreply.github.com>
sreuland added a commit to sreuland/go that referenced this issue Aug 7, 2022
sreuland added a commit to sreuland/go that referenced this issue Aug 7, 2022
sreuland added a commit to sreuland/go that referenced this issue Aug 7, 2022
sreuland added a commit to sreuland/go that referenced this issue Aug 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Protocol 19 Support cap-21, cap-40 and sep-23 in Horizon & SDKs
Projects
No open projects
Status: Done
Development

No branches or pull requests

4 participants