Trait‑Based Architecture for Refunder Service #4051
Conversation
![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 introduces a well-executed trait-based architecture to the refunder crate, significantly improving its testability by decoupling the RefundService from concrete database and blockchain implementations. The changes are clean, well-structured, and accompanied by a comprehensive suite of unit tests. My feedback includes an observation regarding potential inconsistency in order data fetching logic, which is suggested to be addressed in a follow-up PR to maintain separation of concerns.
jmg-duarte
left a comment
jmg-duarte
left a comment
There was a problem hiding this comment.
Great job, this code is so much easier to read!
squadgazzz
left a comment
squadgazzz
left a comment
There was a problem hiding this comment.
I like the PR, but it introduces some complexity and +1k lines of code, so I wanted to better understand the reasoning behind it.
Is there something we can't test with existing e2e tests? Where will unit tests help better? Historically, e2e tests in the services repo have been preferred over unit tests due to a less artificial environment and because unit tests require more time to maintain.
We briefly discussed that unit tests might help alleviate the expensive e2e test runs, so I'd say we can scale up unit tests without compromising CI time as much as with e2e tests. So, in the future, it might suffice to just add a new unit test than to [re]write an e2e test. |
| @@ -0,0 +1,142 @@ | |||
| //! Trait definitions for database and blockchain access. | |||
| #![allow(async_fn_in_trait)] | |||
There was a problem hiding this comment.
Why not using async-trait then?
There was a problem hiding this comment.
I prefer avoiding third-party dependencies where possible. I reckon there are some tradeoffs, but I don't think they play a big role in this case.
|
|
||
| /// Blockchain write operations. | ||
| #[cfg_attr(test, mockall::automock)] | ||
| pub trait ChainWrite: Send + Sync { |
There was a problem hiding this comment.
Does this trait have only one implementation? Do we need this trait then?
There was a problem hiding this comment.
All the new traits have one "production" implementation, plus the one generated by automock.
We need the traits to write more expressive unit tests.
| /// contains UIDs but no order details feels off. Possible fixes: | ||
| /// 1. Skip the submission entirely when `encoded_ethflow_orders` is empty. | ||
| /// 2. Return an error if *all* order‑data lookups fail. | ||
| /// 3. Filter the UID list so it only includes IDs with successful lookups. |
There was a problem hiding this comment.
Hm, maybe that should be the case?
There was a problem hiding this comment.
I got a little confused trying to mirror the refunder’s behavior in this test, and I agree we should make it better.
I left it out of this PR to keep the scope focused, so I believe we can raise it and discuss improvements in a separate issue.
| .get_refundable_orders( | ||
| block_time, | ||
| self.min_validity_duration, | ||
| self.min_price_deviation_bps as f64 / 10_000.0, |
There was a problem hiding this comment.
10_000.0 should probably be extracted into a constant.
Description
This PR follows up on #4029 and introduces a trait‑based architecture for the
refundercrate. By decoupling theRefundServicefrom concrete database and blockchain implementations, we can now write unit tests without relying on (heavyweight) integration tests.Changes
traits.rsmodule that definesDbRead,ChainRead, andChainWritetraits to abstract the two main boundaries of the system.infra/module housing the previous concrete implementations of those traits:AlloyChainimplementsChainReadPostgresimplementsDbReadRefundServicegeneric over those traits, so we can use mocks for unit testing it (and thes real/production implementations) as needed.identify_uids_refunding_statusinto its own function, to simplify testing.RefundStatusenum intotraits.rsso it lives alongside the related abstractions.run()for clearer flow.How to test
Run the unit tests with: