refunder: expand E2E tests #4029
Conversation
Summary of ChangesHello @tilacog, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the testability and coverage of the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly improves the testability and robustness of the refunder crate by introducing trait-based abstractions for database and blockchain interactions. This refactoring enables comprehensive unit testing with mocks, which have been added and are very thorough. Additionally, the E2E test suite has been greatly expanded, covering numerous refund scenarios and edge cases, which increases confidence in the refunder's behavior.
My review focuses on the new abstractions and the refactored logic. The changes are excellent, but I have identified a couple of areas for improvement. One is a potential inconsistency in how refundable orders are processed, which could lead to misleading logs or wasteful transactions. The other is an opportunity to address a TODO left in the code for a performance enhancement. Overall, this is a high-quality contribution that substantially improves the codebase.
| .or_default() | ||
| .push(uid); | ||
| } | ||
| let uid_with_latest_refundablility = futures::future::join_all(futures).await; // TODO: is it worth to switch to FuturesUnordered here? |
There was a problem hiding this comment.
I see you've left a TODO here about switching to FuturesUnordered. This is a good idea for performance, as it would allow processing results as they complete rather than waiting for all futures to finish.
You're already using buffer_unordered from futures::StreamExt in send_out_refunding_tx, which provides similar benefits. You could use it here as well for consistency and improved performance. A concurrency limit (e.g., 10) is a reasonable starting point to avoid overwhelming the RPC endpoint.
| let uid_with_latest_refundablility = futures::future::join_all(futures).await; // TODO: is it worth to switch to FuturesUnordered here? | |
| let uid_with_latest_refundablility: Vec<_> = stream::iter(futures).buffer_unordered(10).collect().await; |
| let futures = uids.iter().map(|uid| { | ||
| let (uid, self_) = (*uid, &self); | ||
| let (uid, database) = (*uid, &self.database); | ||
| async move { | ||
| self_ | ||
| .get_ethflow_data_from_db(&uid) | ||
| database | ||
| .get_ethflow_order_data(&uid) | ||
| .await | ||
| .context(format!("uid {uid:?}")) | ||
| .inspect_err(|err| { | ||
| tracing::error!(?err, ?uid, "failed to get data from db") | ||
| }) | ||
| } | ||
| }); | ||
| let encoded_ethflow_orders: Vec<_> = stream::iter(futures) | ||
| .buffer_unordered(10) | ||
| .filter_map(|result| async { | ||
| result | ||
| .inspect_err(|err| tracing::error!(?err, "failed to get data from db")) | ||
| .ok() | ||
| }) | ||
| .filter_map(|result| async { result.ok() }) | ||
| .collect() | ||
| .await; | ||
| self.submitter | ||
| .submit(uids, encoded_ethflow_orders, contract) | ||
| .submit_refund(&uids, encoded_ethflow_orders, contract) | ||
| .await?; |
There was a problem hiding this comment.
There's a potential issue where uids and encoded_ethflow_orders can become out of sync. If get_ethflow_order_data fails for some UIDs, encoded_ethflow_orders will contain fewer items than the uids slice passed to submit_refund. This could lead to misleading logs and potentially sending wasteful empty transactions if all database lookups fail.
To make this more robust, you could refactor this section to ensure the lists are consistent. The following suggestion filters out orders that fail the database lookup and only attempts to submit a refund if there are successful orders to process.
let futures = uids.iter().map(|uid| async move {
match self.database.get_ethflow_order_data(uid).await {
Ok(data) => Some((*uid, data)),
Err(err) => {
tracing::error!(?err, ?uid, "failed to get data from db");
None
}
}
});
let successful_orders: Vec<_> = stream::iter(futures)
.buffer_unordered(10)
.filter_map(|result| async { result })
.collect()
.await;
if !successful_orders.is_empty() {
let (successful_uids, encoded_ethflow_orders): (Vec<_>, Vec<_>) =
successful_orders.into_iter().unzip();
self.submitter
.submit_refund(&successful_uids, encoded_ethflow_orders, contract)
.await?;
}There was a problem hiding this comment.
I think this is the same problem that’s been reported here:
services/crates/refunder/src/refund_service.rs
Lines 523 to 539 in f79f7d1
services/crates/refunder/src/refund_service.rs
Lines 585 to 608 in f79f7d1
I wasn't aiming for any behavioral changes with this PR, but happy to review and discuss this during the review.
There was a problem hiding this comment.
IMO we should fix in a separate PR
jmg-duarte
left a comment
jmg-duarte
left a comment
There was a problem hiding this comment.
IMO & ideally, this should be broken into two parts: the refactor and the tests
Would make review much easier.
Left some comments
| let futures = uids.iter().map(|uid| { | ||
| let (uid, self_) = (*uid, &self); | ||
| let (uid, database) = (*uid, &self.database); | ||
| async move { | ||
| self_ | ||
| .get_ethflow_data_from_db(&uid) | ||
| database | ||
| .get_ethflow_order_data(&uid) | ||
| .await | ||
| .context(format!("uid {uid:?}")) | ||
| .inspect_err(|err| { | ||
| tracing::error!(?err, ?uid, "failed to get data from db") | ||
| }) | ||
| } | ||
| }); | ||
| let encoded_ethflow_orders: Vec<_> = stream::iter(futures) | ||
| .buffer_unordered(10) | ||
| .filter_map(|result| async { | ||
| result | ||
| .inspect_err(|err| tracing::error!(?err, "failed to get data from db")) | ||
| .ok() | ||
| }) | ||
| .filter_map(|result| async { result.ok() }) | ||
| .collect() | ||
| .await; | ||
| self.submitter | ||
| .submit(uids, encoded_ethflow_orders, contract) | ||
| .submit_refund(&uids, encoded_ethflow_orders, contract) | ||
| .await?; |
There was a problem hiding this comment.
IMO we should fix in a separate PR
This commit reverts the trait-based refactoring in the refunder crate to facilitate code review by splitting the work across two PRs. The trait-based architecture will be re-introduced in a follow-up PR.
This reverts commit b6b7d8c. Also adds the "rand" feature to the shared crate, resolving the build error.
…sts--pr # Conflicts: # crates/shared/Cargo.toml # crates/shared/src/sources/balancer_v2/pool_fetching/registry.rs
tilacog
changed the title
|
For future reference, I've reverted the commit f098c48 to split this PR in two. It'll be reintroduced in a follow-up PR. |
| owner if owner == NO_OWNER => Self::Invalid, | ||
| owner if owner == INVALIDATED_OWNER => Self::Refunded, |
There was a problem hiding this comment.
this should work
| owner if owner == NO_OWNER => Self::Invalid, | |
| owner if owner == INVALIDATED_OWNER => Self::Refunded, | |
| NO_OWNER => Self::Invalid, | |
| INVALIDATED_OWNER => Self::Refunded, |
crates/e2e/tests/e2e/refunder.rs
Outdated
| /// Alternatives to leaking inlcude: | ||
| /// - Transmuting, but requires unsafe and introduces some drop complexity | ||
| /// - Borrow services & onchain structs | ||
| /// - Use Rc/Arc |
There was a problem hiding this comment.
To be honest, I prefer the Arc solution, I find that the leak may be a footgun
There was a problem hiding this comment.
Agree. Why not simply use Arc?
There was a problem hiding this comment.
After giving it a bit more thought, it seems we can't use Arc/Rc here because RefunderTestSetup> struct can't hold both Services<'a>, which in turn borrows &'a Onchain.
However, if we were to modify the Services and Onchain structs themselves, I think we could use Arc/Rc, but that seems like a PR on its own.
In the meantime, I managed to get it working quite easily using ouroboros for the self-referencing part.
What do you guys think? Should I stick with ouroboros, or do we want to refactor Services/Onchain to use Arc/Rc instead?
There was a problem hiding this comment.
I know this is probably now what you want to read but IMO we should instead (if possible):
- refactor the test to match the structure of the other ones (that don't require these tricks — read ouroboros, leaks, etc)
- fix the E2E testing structure in general (if there's space for it) cc'ing @extrawurst for discussion/visibility
- since it would tackle a big part of the tech debt
- at the same time we could shift the E2E specialization from Ilya to you since you'd literally be learning and re-writing them
Fixing the E2E doesn't need to be wholesale, it can be similar to the alloy migration, bit by bit, and with E2E there's no risk of adding new tests and infrastructure for said tests and break previous ones; so we could do new proper structures and migrate the E2E tests bit by bit
I suggest this because:
- while the
Box::leakusage in tests is most likely ok, it's a footgun waiting to blow - ouroboros would fix this now but we'd add more tech debt to our existing one
- if you're eventually adding traits etc (like in the first iteration of the PR), it's just a matter of time until you hit this again -> adding more leak/ouroboros -> meaning more debt
There was a problem hiding this comment.
100% agree.
I'm working on removing the setup struct entirely, so tests will follow the e2e pattern and we won't bump into this issue anymore.
There was a problem hiding this comment.
I removed the helper/offending struct in the last commit (bb5233f) and replaced it with a builder that just borrows both Services and Onchain, so there's no more lifetime wrangling. Hopefully that makes more sense now.
squadgazzz
left a comment
squadgazzz
left a comment
There was a problem hiding this comment.
Amazing PR 🚀
Just a few clarification questions before approving it.
| // Tests that orders with slippage below, at, or above the min_price_deviation | ||
| // threshold are refunded according to the SQL >= check. |
There was a problem hiding this comment.
Does the result depend only on a SQL query? If so, can this be tested in a more lightweight setup? Maybe expanding the database tests would be enough here? And keep only 1-2 cases with a local node involved. My worry is that each test executes in 5-7s, which adds ~40s to the CI job execution. No strong opinion tho. Especially, if the alternative solution will require almost the same time to execute.
There was a problem hiding this comment.
Fair point.
Considering SQL alone, then I believe yes. I can see order expiration and slippage being tested at the postgres_refundable_ethflow_orders test
The current test operates one layer above and verifies the full integration, so I dare to say there's a bit more of value being added here.
Nonetheless, I'm still not sure what's the best way forward here.
Maybe we can remove the boundary test cases, since they are already covered in the database tests?
There was a problem hiding this comment.
I mean, we don't need to drop all the test cases, we should keep at least 1. But for example, if changing some test case params only depends on the SQL query output, we should test it in the DB tests to avoid spinning up the protocol, etc, just to ensure the query returns a reasonable result. If changing the test case params involves some other logic on the refunder side, then we should keep everything as is for sure.
There was a problem hiding this comment.
I’m on the fence about this. The current build (bb5233f) tests three scenarios for each threshold:
- Refund should be triggered
- Refund should not be triggered
- Edge case, outcome depends on the specific attribute
I'm OK about dropping the edge cases, but ideally we should still test both the triggering and non-triggering paths...
I'm totally with you on the impact this has on our overall test time though, so I'm not sure how to move forward here.
There was a problem hiding this comment.
That said, I think we could get the same confidence about triggering behavior from unit tests, which would be way lighter. We'd need some refactoring involving new traits to get there, but once we have them it should be much better. Do you think it's OK to leave these as they are for now and revisit once we have the unit tests in place?
There was a problem hiding this comment.
Let's leave it as is then. If this requires additional work, I think we can live with an extra 30s of CI running.
| .create_and_index_ethflow_order(slippage.order, validity.order) | ||
| .await; | ||
|
|
||
| advance_time_past_expiration(&web3, valid_to).await; |
There was a problem hiding this comment.
Not sure I understand what stops the order from being settled. Could you add a comment in the code?
There was a problem hiding this comment.
Currently, settlement won't happen because of the time distance between the chain (2020) and autopilot (2026, current).
All tests' orders are expired at creation time by default because they are created based on chain time. The exception being the refunder_skips_settled_orders test, which manually creates valid orders.
I'll document this more thoroughly 👍
There was a problem hiding this comment.
I hope the last version (bb5233f) is sufficiently documented, please let me know if I should expand/elaborate more.
tilacog
force-pushed
the
tiago/refunder-e2e-tests
branch
from
e696c98 to
bb5233f
Compare
|
(force pushed just the branch tip, only modified the |
jmg-duarte
left a comment
jmg-duarte
left a comment
There was a problem hiding this comment.
I find the changes from invalidated to refunded kind of confusing
crates/e2e/tests/e2e/refunder.rs
Outdated
| /// | ||
| /// Returns: (ExtendedEthFlowOrder, OrderUid, valid_to timestamp) |
There was a problem hiding this comment.
This doesn't add information one isn't getting from the function signature
crates/e2e/tests/e2e/refunder.rs
Outdated
| /// For tests requiring actual settlement, use `Utc::now()` for `valid_to`. | ||
| /// See `refunder_skips_settled_orders` for an example. |
There was a problem hiding this comment.
I feel that this should be part of the mod documentation as it pertains to how to writing tests, not attached to a single function
crates/e2e/tests/e2e/refunder.rs
Outdated
| let ethflow_order_2 = | ||
| ExtendedEthFlowOrder::from_quote("e_response, valid_to).include_slippage_bps(9999); | ||
| ) | ||
| .with_slippage_bps(slippage.order) // Explicit: testing this SQL filter |
There was a problem hiding this comment.
I'm confused as to what this comment means, you're testing this filter?
There was a problem hiding this comment.
Possibly a leftover comment from a previous version, my bad.
Removed in 5c493b2
crates/e2e/tests/e2e/refunder.rs
Outdated
| // Verify order is not yet invalidated | ||
| assert_ne!( | ||
| ethflow_order | ||
| .status(onchain.contracts(), ethflow_contract) | ||
| .await, | ||
| RefundStatus::Refunded | ||
| ); |
There was a problem hiding this comment.
The docstring kind of mismatches the code, it's checking if the order wasn't refunded yet, meaning it can actually be invalid.
If you're expecting the order to not be invalid, you should check for that instead (?)
There was a problem hiding this comment.
Bad wording on my end, and also poor logic.
What we should be testing here can be reworded to "checking if the order is still eligible for refund (not yet reimbursed)" or "check if order has not been refunded yet".
Fixed the comment in a760a89
| assert_eq!( | ||
| ethflow_order | ||
| .status(onchain.contracts(), ethflow_contract) | ||
| .await, | ||
| RefundStatus::Refunded, | ||
| "Order should already be invalidated by user" | ||
| ); |
There was a problem hiding this comment.
If the order is invalidated on chain, why are you checking against refunded?
There was a problem hiding this comment.
The RefundStatus::Refunded variant is a bit confusing because it's semantic overloaded. It represents both:
- Orders refunded by the refunder service, and
- Orders invalidated/cancelled by the user
So, Refunded is the variant we have for a refund in a terminal state.
services/crates/refunder/src/refund_service.rs
Lines 40 to 61 in bb5233f
Note: The impl From above was originally at crates/e2e/tests/e2e/ethflow.rs#L796-L804 in the main branch, before I consolidated the EthFlowOrderOnchainStatus enum w/ the RefundStatus I took from test code. I tried to not loose anything in translation, but now I'm having second thoughts.
Not sure how to go from here. Should I add more comments to explain this further? I'd rather avoid expanding/specializing the variants in this PR.
There was a problem hiding this comment.
(Not sure I'm missing something here too)
| // The order should still be invalidated (status unchanged) | ||
| assert_eq!( | ||
| ethflow_order | ||
| .status(onchain.contracts(), ethflow_contract) | ||
| .await, | ||
| RefundStatus::Refunded | ||
| ); |
| assert_ne!( | ||
| ethflow_order | ||
| .status(onchain.contracts(), ethflow_contract) | ||
| .await, | ||
| RefundStatus::Refunded, | ||
| "Refunder should not have invalidated the already-settled order" | ||
| ); |
There was a problem hiding this comment.
There doesn't seem to be a proper status for settled orders. Should this actually be invalid in the face of the refunder since it can't be refunded?
Resolves: #3785
Description
This PR improves test coverage for the refunder crate by expanding the end-to-end tests covering many refund scenarios.
Other Changes
RefundStatusfrom tests to the main crate logic.sharedcrate that was blocking this work.Run the refunder E2E tests: