chore: refactor SwapsController so it extends from BaseControllerV2 #25681
Conversation
…tension into swaps/controller-v2
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #25681 +/- ##
===========================================
+ Coverage 69.77% 69.97% +0.20%
===========================================
Files 1376 1390 +14
Lines 48403 48958 +555
Branches 13348 13460 +112
===========================================
+ Hits 33773 34258 +485
- Misses 14630 14700 +70 ☔ View full report in Codecov by Sentry. |
nikoferro
changed the title
Builds ready [468c385]
Page Load Metrics (62 ± 9 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
|
Manually Tested Mainnet OP Stack |
Co-authored-by: Jongsun Suh <jongsun.suh@icloud.com>
Builds ready [b5ecb14]
Page Load Metrics (77 ± 20 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
| // Private Methods | ||
| private async _fetchSwapsNetworkConfig(chainId: ChainId) { |
There was a problem hiding this comment.
nit: Could we rename all private properties to consistently follow the hash-prefix naming scheme?
The hash prefix is enforced by JavaScript at runtime on the syntax level, whereas the scope keywords public/protected/private are compile-time syntactic sugar that aren't enforced (we also find it unnecessary to use the public keyword for this reason).
| // Private Methods | |
| private async _fetchSwapsNetworkConfig(chainId: ChainId) { | |
| // Private Methods | |
| async #fetchSwapsNetworkConfig(chainId: ChainId) { |
There was a problem hiding this comment.
I considered this during the refactor, but the problem we have is that we need to spy on "internal" methods for testing purposes. So I cannot completely enforce privacy at runtime (at least not at the moment).
The best compromise i found was to label methods as private or public so we can check this with TypeScript.
Ideally we should redesign the controller data flow so we care only about public methods output and test only that. But for now this involves work that goes a bit out of the scope of this migration to BaseController V2
There was a problem hiding this comment.
Oh you're right looks like there are quite a few of those spyOn calls. I'm good with refactoring the tests in a separate ticket.
|
|
||
| export default class SwapsController extends BaseController< |
There was a problem hiding this comment.
Probably out-of-scope for this PR, but since this controller contains polling logic, could it be refactored to inherit from StaticIntervalPollingController in the future?
There was a problem hiding this comment.
Thanks for the heads up! I didn't know we had that. I see that TokenDetectionController is using it this way also. Like you said, i consider this a out of scope for this PR, but will definitively will add this on our to-do list
Builds ready [b8b7ff7]
Page Load Metrics (219 ± 218 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
| @@ -32,7 +37,7 @@ const TEST_AGG_ID_6 = 'TEST_AGG_6'; | |||
| const TEST_AGG_ID_BEST = 'TEST_AGG_BEST'; | |||
| const TEST_AGG_ID_APPROVAL = 'TEST_AGG_APPROVAL'; | |||
|
|
|||
| const POLLING_TIMEOUT = SECOND * 1000; | |||
| // const POLLING_TIMEOUT = SECOND * 1000; | |||
There was a problem hiding this comment.
Do we need to keep this here?
There was a problem hiding this comment.
its part of the todo list, kept it as a comment so the linter does not complain, but i did want to keep those values around for when we refactor to extend the polling functionality from StaticIntervalPollingController
| lastEthersProviderChainId, | ||
| ); | ||
| }); | ||
| // TODO: Re think how to test this without exposing internal state |
There was a problem hiding this comment.
Does this need to be finished?
There was a problem hiding this comment.
not on this PR, the current design involves testing functionality that should be internal. ideally we need to separate internal logic from the public one, and only test methods that we expose as public. for this a much bigger refactor is needed and i already added a jira task for that
|
Builds ready [113e8eb]
Page Load Metrics (63 ± 6 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
metamaskbot
added
the
release-12.3.0
Description
This PR refactors SwapsController so it extends from BaseController (v2). This refactor also involves migrating the current set of unit tests to Typescript
Related issues
Fixes:
Manual testing steps
All tests are in the green, but the best approach would be test swaps manually end to end.
Screenshots/Recordings
No visual impact
Before
N/A
After
N/A
Pre-merge author checklist
Pre-merge reviewer checklist