Conversation
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
All alerts resolved. Learn more about Socket for GitHub. This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. Ignoring alerts on:
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #29141 +/- ##
===========================================
- Coverage 82.23% 56.91% -25.32%
===========================================
Files 5107 5117 +10
Lines 134974 135407 +433
Branches 30358 30447 +89
===========================================
- Hits 110997 77071 -33926
- Misses 16410 52082 +35672
+ Partials 7567 6254 -1313 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Jwhiles
force-pushed
the
musd-439
branch
5 times, most recently
from
c367de5 to
e2b61f5
Compare
Jwhiles
force-pushed
the
musd-439
branch
2 times, most recently
from
9ab1dac to
1b2d591
Compare
| useFocusEffect( | ||
| useCallback(() => { | ||
| dispatch(upgradeMoneyAccount()); | ||
| }, [dispatch]), | ||
| ); |
There was a problem hiding this comment.
@Jwhiles Just thinking out loud here. What about wrapping the Money navigation stack in a function that upgrades the Money Account? This way when the user is redirected to any of the Money routes we upgrade the account if needed. For example, when the user is in the onboarding carousel we could upgrade the account behind the scenes so that this is done by the time they visit the Money Home screen?
I haven't done this before but wondering if something like this would work?
If this is overkill please push back! Just an idea 😄
function MoneyScreenStack() {
return (
<Stack.Navigator>
<Stack.Screen ... />
<Stack.Screen ... />
</Stack.Navigator>
);
}
// Middleware that wraps Money routes
function MoneyAccountStackGate() {
// Upgrade if necessary
useEffect(
useCallback(() => {
dispatch(upgradeMoneyAccount());
}, [dispatch]),
);
return <MoneyScreenStack />;
}
// In MainNavigator.js register the MoneyAccountStackGate
{isMoneyHomeScreenEnabled && (
<>
<Stack.Screen
name={Routes.MONEY.ROOT}
component={MoneyAccountStackGate}
options={{ headerShown: false, ...slideFromRightAnimation }}
/>
</>
)}We could also just wrap our navigation stack with a <MoneyAccountUpgradeProvider />
There was a problem hiding this comment.
Yeah this is a nice approach! Thanks for the suggestion :)
| @@ -0,0 +1,25 @@ | |||
| import { object, string, type Infer, create } from '@metamask/superstruct'; | |||
There was a problem hiding this comment.
Should we set the Earn team as the codeowners for these new files?
| const featureState = initMessenger.call( | ||
| 'RemoteFeatureFlagController:getState', | ||
| ); | ||
| const chompApiConfig = parseChompApiConfig( | ||
| featureState.remoteFeatureFlags?.earnChompApiConfig, | ||
| ); |
There was a problem hiding this comment.
Does this access the RemoteFeatureFlagController state before it has a chance to fetch latest flags? I'm wondering if it's possible that we init with a potentially stale config if we ever update earnChompApiConfig in LaunchDarkly.
There was a problem hiding this comment.
hmm yeah possibly. I could take a similar approach to the money controller, and actually 'initialise' it when the user unlocks?
There was a problem hiding this comment.
@Matt561 I thought about this some more, and I think fixing it requires us updating the chomp service. I'm up for doing that, but if it's okay with you - I'll do it in a follow up PR - as I'd like to get this one merged.
|
Works for me (apart from the 401s) |
cortisiko
left a comment
cortisiko
left a comment
There was a problem hiding this comment.
there are failing e2e, feel free to ping once those are passing!
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f6afb9b. Configure here.
🔍 Smart E2E Test Selection
click to see 🤖 AI reasoning detailsE2E Test Selection: Performance Test Selection: |
|
|
Can we solve and investigate if it's okay to go with the library mentioned by socket security? |
Hey Tomas, thanks for calling it out - I created this package, and it uses fetch because it exists to talk to our new chomp API. I'm going to mark it as an acceptable risk :) |
|
@SocketSecurity ignore npm/@metamask/chomp-api-service@3.0.0 |
metamaskbotv2
Bot
added
the
release-7.77.0
6 participants
Description
Changelog
CHANGELOG entry: Add chomp-api-service and money-account-upgrade-controller - initialise money account upgrade process when user visits the money account route.
Related issues
Fixes:
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Performance checks (if applicable)
trace()for usage andaddTokenfor an exampleFor performance guidelines and tooling, see the Performance Guide.
Pre-merge reviewer checklist
Note
Medium Risk
Adds new Engine services/controllers that bootstrap on keyring unlock and triggers an account upgrade flow on Money navigation, increasing risk of unexpected network calls or upgrade attempts. Changes touch Engine initialization/state wiring and navigation entrypoints but are guarded with logging, readiness checks, and deduping.
Overview
Automatically kicks off the Money account upgrade flow when the user navigates to the Money home by introducing
MoneyAccountStackGate, which dispatches the newupgradeMoneyAccountthunk on mount.Adds
ChompApiService(configured via the new remote flagearnChompApiConfigwith a dev-URL fallback) and aMoneyAccountUpgradeControllerthat bootstraps after keyring unlock by fetching CHOMP service details and initializing upgrade parameters.Wires both new clients into
Engine(init, messengers, types, state fixtures, and background state events), updates E2E API mocks to cover the CHOMP service-details endpoint, and adds unit tests for the new init and upgrade behaviors.Reviewed by Cursor Bugbot for commit 64bdcb7. Bugbot is set up for automated code reviews on this repo. Configure here.