Skip to content

Conversation

@kmchicoine
Copy link
Contributor

@kmchicoine kmchicoine commented Oct 28, 2025

  • New e2e test crate: Adds end-to-end tests for the TIPS ingress RPC service, validating transaction and bundle submission flows against a running server
  • Testing infrastructure: Includes JSON-RPC client, transaction signing fixtures, and helper utilities for creating test bundles and signers
  • Test coverage: Tests empty/invalid input handling, single transactions, multi-transaction bundles, and bundle replacement with UUIDs
  • Test gating: Tests require full TIPS infrastructure, so they require INTEGRATION_TESTS=1 environment variable set

gas_limit: u64,
gas_price: u128,
) -> Result<Bytes> {
let tx = TxLegacy {
Copy link
Contributor

Choose a reason for hiding this comment

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

just curious, any reason for TxLegacy over TxEip1559?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great question, primarily because it had fewer arguments and was easier to work with initially. I'm about to push a change to fix the broken tests, so I can swap over to TxEip1559 (or if we want multiple types of transaction builders, happy to keep the TxLegacy and add TxEip1559 and subsequent tests)

Copy link
Contributor

Choose a reason for hiding this comment

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

sounds good! i think we can leave txlegacy for now and then if we actually need txeip1559 later we can just swap it out in a follow up

Copy link
Collaborator

Choose a reason for hiding this comment

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

Would suggest we switch to 1559 if you're in the area, most transactions we get are 1559 and it'll help validate fee ordering etc.

@kmchicoine kmchicoine self-assigned this Oct 31, 2025
data: Option<serde_json::Value>,
}

impl TipsRpcClient {
Copy link
Collaborator

Choose a reason for hiding this comment

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

For this can we use a alloy provider? If it doesn't support eth_sendBundle could we upstream that?

gas_limit: u64,
gas_price: u128,
) -> Result<Bytes> {
let tx = TxLegacy {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would suggest we switch to 1559 if you're in the area, most transactions we get are 1559 and it'll help validate fee ordering etc.


```bash
cd tips
INTEGRATION_TESTS=1 cargo test --package tips-e2e-tests -- --nocapture
Copy link
Contributor

Choose a reason for hiding this comment

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

qq to confirm: if i make a change to the implementation (not the test), i'll still need to re-run just start-all right?

@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the Stale label Dec 16, 2025
@github-actions github-actions bot removed the Stale label Dec 19, 2025
Copy link
Contributor

@wlawt wlawt left a comment

Choose a reason for hiding this comment

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

LGTM!

i wonder if we should add a command to the justfile like just e2e that'll execute INTEGRATION_TESTS=1 cargo test --package tips-system-tests --test integration_tests?

And also echo a reminder message to have the infra running locally, since technically tests can still fail if we have INTEGRATION_TESTS=1 but no infra running, and that might create some confusion. WDYT?

@kmchicoine
Copy link
Contributor Author

LGTM!

i wonder if we should add a command to the justfile like just e2e that'll execute INTEGRATION_TESTS=1 cargo test --package tips-system-tests --test integration_tests?

And also echo a reminder message to have the infra running locally, since technically tests can still fail if we have INTEGRATION_TESTS=1 but no infra running, and that might create some confusion. WDYT?

Love it- I'll also see if I can add something to the error messages on the integration tests as a reminder to have the infra running

@kmchicoine kmchicoine merged commit 2f8b0ff into master Dec 19, 2025
10 checks passed
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.

4 participants