Create shield controller #6137
Create shield controller #6137
Conversation
|
No dependency changes detected. Learn more about Socket for GitHub. 👍 No dependency changes detected in pull request |
matthiasgeihs
force-pushed
the
mg/create-shield-controller
branch
from
8e91db5 to
a6d65ff
Compare
matthiasgeihs
force-pushed
the
mg/create-shield-controller
branch
2 times, most recently
from
02bf49f to
a9bf413
Compare
matthiasgeihs
force-pushed
the
mg/create-shield-controller
branch
2 times, most recently
from
d2a3e58 to
258c03b
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
matthiasgeihs
force-pushed
the
mg/create-shield-controller
branch
from
6c7f901 to
99a4963
Compare
| // eslint-disable-next-line no-unmodified-loop-condition | ||
| while (!timeoutReached) { | ||
| const startTime = Date.now(); | ||
| const res = await fetch(`${this.#baseUrl}/api/v1/coverage/result`, { |
There was a problem hiding this comment.
This file seems to rely on the global fetch function. We've had problems with this in the past across clients, and we've found it's better to pass in the fetch function so that clients can pass whatever implementation is specific to the platform. Thoughts on having your ShieldRemoteBackend class take a fetch option? (Alternatively, the "data service" pattern I mention in an earlier comment already does this.)
There was a problem hiding this comment.
I'm ok with passing in an optional fetch function, if this appeared useful in the past.
For context: I've been following the approach of TransactionController here and it seems like it's just relying on the default fetch as well.
I'll address the other minor comments first, and then potentially come back to this one, including your ask to use the "data service" pattern.
There was a problem hiding this comment.
Can you say a bit more about what problems you were previously facing with using fetch? It seems to work fine in the context of TransactionController. Anyhow, I can see that relying on platform-dependent globals can be an issue. Added a fetch parameter.
There was a problem hiding this comment.
Can you say a bit more about what problems you were previously facing with using
fetch?
I believe that we last encountered issues with fetch in 2023, and at the time, Mobile was using Node 14 in development, and we were using a version of React Native that didn't support Node 16 (and perhaps tools like LavaMoat didn't support Node 16 either). Therefore we couldn't assume that global fetch worked in all platforms. I believe we tried to resolve the differences using cross-fetch, but there were security issues with that package. Simliarly, node-fetch didn't play nice with Mobile and isomorphic-fetch didn't play nice with Snaps. I think we ultimately decided that injecting fetch was the most bulletproof path going forward. (Here is a PR removing node-fetch from a repo and here is a PR removing isomorphic-fetch, with related Slack thread).
However, since then, various parts of our stack has been upgraded (dev version of Node on both platforms, React Native in Mobile, LavaMoat, etc.). In fact I see that Snaps is actively using global fetch here, and of course you point out that TransactionController uses it. So perhaps the advice I gave you is outdated. I'll look into this more tomorrow.
- module logger - fix comment - add function doc - modify state in update callback - unexport allowed actions and events - export controller actions and events
matthiasgeihs
force-pushed
the
mg/create-shield-controller
branch
from
6d2343e to
7accf4b
Compare
Explanation
New controller for MetaMask Shield.
Functionality:
References
Changelog
Checklist
I've communicated my changes to consumers by updating changelogs for packages I've changed, highlighting breaking changes as necessaryI've prepared draft pull requests for clients and consumer packages to resolve any breaking changes(The last two are not applicable as this is a new package.)