fix: fixes how a Contract instance is created with web3 v4, also increased coverage for the controller #241
Conversation
| }, | ||
| ); | ||
| }); | ||
| const allowancePromise = async () => { |
There was a problem hiding this comment.
(Unrelated review observation, not to be addressed in this PR): Probably want some error handling here?
There was a problem hiding this comment.
@legobeat initially i added a try/catch in here but the linter was complaining that it was unnecessary. Which i actually think it makes sense for this to be labeled as unnecessary since if either the timeout throws, allowancePromise throws, it is expected for the function to fail and does not need an special error handling. This will end up being managed by the fetchQuotes catch clause
| const result: bigint = await contract.methods | ||
| .allowance(walletAddress, getSwapsContractAddress(this.config.chainId)) | ||
| .call(); | ||
| return Number(result); |
There was a problem hiding this comment.
(Unrelated review observation, not to be addressed in this PR): Could look at if it's possible to return some bignumber/decimal type or even string without going through Number, to remove the potential factor of precision-loss and floating-point rounding errors.
There was a problem hiding this comment.
@legobeat i agree with your observation. This was done just to match the typings. Also if you check line 742, the allowance is still converted to Number, and since this is the way it has been working the last 3 years, i didn't want to include this change and potentially add an issue for something that was already out of the scope of this PR
legobeat
requested review from
a team,
dan437,
forest-diggs-consensys,
rekmarks and
wachunei
infiniteflower
left a comment
infiniteflower
left a comment
There was a problem hiding this comment.
Good to see that we are improving type safety, looking forward to getting rid of as many any as we can.
## **Description** - Upgrade `@metamask/swaps-controller` from v8 to v9 - Remove redundant patches Resolves a bunch of outstanding legacy, including `web-provider-engine`. Also enables Base Network for Swaps. ## **Related issues** - #9286 `@metamask/swaps-controller` - v8 upgrade - #9612 - reverted v8 upgrade - MetaMask/swaps-controller#241 - fixed issue that motivated revert of v8 upgrade #### Blocked by - [x] MetaMask/swaps-controller#245 - [x] MetaMask/swaps-controller#219 This PR also patches `swaps-controller@9.0.0` to fix the `web3` import that works differently between the test environment and the actual mobile app / E2E test environment. This should be removed once a `swaps-controller` version addressing this is published. (MetaMask/swaps-controller#255) ## **Manual testing steps** Regression over swaps functionality in the 4 different scenarios: ETH -> WETH WETH -> ETH ETH -> ERC20 ERC20 -> ETH ## **Screenshots/Recordings** ### **Before** ### **After**     ## **Pre-merge author checklist** - [x] I’ve followed [MetaMask Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [ ] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [x] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. --------- Co-authored-by: nikoferro <nicolaspatricioferro@gmail.com>
This PR attempts to fix the issue that prompted the following revert: MetaMask/metamask-mobile#9612
The issue would still have been undetected under a 100% coverage situation, due to the strategy used to test the controller. Since right now we are mocking every fetch/request, we are also faking the web3 instance. Using the current strategy, the private method
getERC20Allowancewould never have been called.