Conversation
| gas_limit: u64, | ||
| gas_price: u128, | ||
| ) -> Result<Bytes> { | ||
| let tx = TxLegacy { |
There was a problem hiding this comment.
just curious, any reason for TxLegacy over TxEip1559?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
| data: Option<serde_json::Value>, | ||
| } | ||
|
|
||
| impl TipsRpcClient { |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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.
crates/e2e-tests/README.md
Outdated
|
|
||
| ```bash | ||
| cd tips | ||
| INTEGRATION_TESTS=1 cargo test --package tips-e2e-tests -- --nocapture |
There was a problem hiding this comment.
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?
|
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. |
wlawt
left a comment
wlawt
left a comment
There was a problem hiding this comment.
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 |
3 participants
Uh oh!
There was an error while loading. Please reload this page.