Conversation
|
Warning MetaMask internal reviewing guidelines:
|
| const MAX_UINT256 = | ||
| '115792089237316195423570985008687907853269984665640564039457584007913129639935'; | ||
|
|
||
| // Sourced from https://github.com/MetaMask/snap-cash-account-poc/blob/70709e15ddc56288dd9eefa45b425a756f25d2fb/packages/snap/src/api/config.ts#L39-L40 |
There was a problem hiding this comment.
not sure if this the correct address here - I sourced this from the POC - but it might be that this value should also come from the chomp service details endpoint
33e853a to
16087d2
Compare
| to: delegateAddress, | ||
| caveats: [ | ||
| { type: 'redeemer', redeemers: [vedaVaultAdapterAddress] }, | ||
| { type: 'valueLte', maxValue: 0n }, |
There was a problem hiding this comment.
So if you are using scope then valueLte will automatically be added to the delegation. No need to specify it here.
But maybe a better way would be to not use @metamask/smart-accounts-kit which is a bit bigger package but use @metamask/delegation-core to construct the delegation. Its should still be simple enough but will have much less dependencies then SAK.
There was a problem hiding this comment.
I think here is an example of using delegation core: https://github.com/MetaMask/metamask-extension/pull/41809/changes
There was a problem hiding this comment.
Ow and to get addresses you can use this package: https://github.com/MetaMask/smart-accounts-kit/tree/main/packages/delegation-deployments
| ); | ||
| } | ||
|
|
||
| return 'completed'; |
There was a problem hiding this comment.
we need to also store the delegation then into user authenticated storate + send it to chomp API and intent (delegation hash and some metadata). Not sure if that is means as part of another step.
55a57a7 to
c4c6220
Compare
|
@metamaskbot publish-preview |
|
@metamaskbot publish-preview |
|
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
|
@metamaskbot publish-preview |
|
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
|
|
||
| - **BREAKING:** The controller messenger now requires access to six additional allowed actions: `AuthenticatedUserStorageService:listDelegations`, `AuthenticatedUserStorageService:createDelegation`, `ChompApiService:verifyDelegation`, `ChompApiService:getIntentsByAddress`, `ChompApiService:createIntents`, and `DelegationController:signDelegation`. Delegation signing is now delegated to `@metamask/delegation-controller` rather than calling `KeyringController:signTypedMessage` directly; consumers must instantiate `DelegationController` and update their messenger configuration accordingly. ([#8621](https://github.com/MetaMask/core/pull/8621)) | ||
| - **BREAKING:** `init()` now takes a `{ chainId, boringVaultAddress }` object instead of an `InitConfig`. The EIP-7702 delegator implementation and caveat enforcer addresses are resolved from `@metamask/delegation-deployments` for the target chain; `init()` throws if the chain is not supported by Delegation Framework 1.3.0. The `InitConfig` type is no longer exported. ([#8621](https://github.com/MetaMask/core/pull/8621)) | ||
| - **BREAKING:** `UpgradeConfig` no longer includes `musdTokenAddress` (now derived internally from the Veda protocol service details). ([#8621](https://github.com/MetaMask/core/pull/8621)) |
There was a problem hiding this comment.
The UpgradeConfig still has mUSD?
There was a problem hiding this comment.
Sorry, this was leftover from an earlier iteration - will clear up the changelog.
|
@metamaskbot publish-preview |
|
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
|
@metamaskbot publish-preview |
|
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
Adds a third step to the upgrade sequence that builds, signs, and submits the auto-deposit delegation that authorises CHOMP's delegate to move mUSD into the Veda vault on the user's behalf. The step: - Looks up existing delegations via AuthenticatedUserStorageService:listDelegations and skips when one matches the configured (delegator, delegate, chain, token). - Builds a per-call 32-byte salt and constructs the delegation with redeemer + valueLte + erc20TransferAmount caveats. - Signs as EIP-712 V4 typed data via KeyringController:signTypedMessage. - Submits to ChompApiService:verifyDelegation; throws on rejection. The `InitConfig` passed to `init()` carries the delegator-impl and caveat-enforcer addresses; the messenger gains the three new allowed actions.
…yments Replaces the @metamask/smart-accounts-kit dependency with the lower-level @metamask/delegation-core (caveat-term encoders) and @metamask/delegation-deployments (Delegation Framework contract registry). - `init()` now takes only a chainId; the EIP-7702 delegator-impl and caveat-enforcer addresses are resolved from `DELEGATOR_CONTRACTS['1.3.0'][chainId]` rather than being passed in. `InitConfig` is no longer exported. - The build-delegation step builds the three caveats directly with delegation-core's `createERC20TransferAmountTerms` / `createValueLteTerms` / `createRedeemerTerms`, and constructs the EIP-712 typed-data message inline (13 lines). - Drops a duplicate `valueLteEnforcer` caveat that the smart-accounts-kit `erc20TransferAmount` scope helper was inadvertently appending on top of the explicit one we passed in. Net dependency size: ~3 MB → ~650 kB. No behaviour change beyond the duplicate-caveat fix.
Hands off delegation signing (and DelegationManager address resolution) to @metamask/delegation-controller, which the wallet client already wires up globally with a `getDelegationEnvironment` callback. - Adds `@metamask/delegation-controller` as a dependency. - Swaps `KeyringController:signTypedMessage` for `DelegationController:signDelegation` in the messenger allowlist. - Drops `delegationManager` from `UpgradeConfig` / `StepContext`; this controller no longer needs to know the DelegationManager address — DelegationController resolves it. - Removes the inlined `SIGNABLE_DELEGATION_TYPED_DATA` and salt hex→bigint conversion from build-delegations (~25 lines). The build-delegation step still resolves enforcer + EIP-7702 impl addresses from `@metamask/delegation-deployments` directly, since those are statically typed and DelegationController only exposes them via a string-keyed bag.
The build-delegation step now signs two delegations per upgrade — one authorising transfers of mUSD (deposit-side) and one authorising transfers of the Veda boring vault share token vmUSD (withdrawal-side). Both delegations share delegator, delegate, and redeemer (the Veda vault adapter); only the ERC20TransferAmount caveat's token differs. The "already-done" check runs per-token, so re-running the upgrade after a partial failure only re-signs the missing delegation. Signing is sequential, deposit before withdrawal, so the user sees one prompt at a time. The withdrawal-side token is the Veda boring vault contract address. This is hardcoded per chain in the controller (mainnet only) until the CHOMP service-details API exposes it; misconfigured chains throw at init() time.
After CHOMP verifies a delegation, the build-delegation step now also calls AuthenticatedUserStorageService:createDelegation so the signed delegation is stored against the user's profile. Without this the listDelegations matcher on the next run would never find a stored record and we'd re-sign on every upgrade attempt. Order is verify-then-store: if storage fails after CHOMP verification, nothing is persisted and the next run rebuilds from scratch with a fresh salt. The inverse (store-then-verify) would risk persisting a delegation CHOMP later rejects. Metadata records the per-token symbol (mUSD / vmUSD), the cash-deposit / cash-withdrawal intent type, MAX_UINT256 as the allowance, and a delegationHash derived from @metamask/delegation-core's hashDelegation.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…Mask#30002) <!-- Please submit this PR as a draft initially. Do not mark it as "Ready for review" until this PR meets the canonical Definition of Ready For Review in `docs/readme/ready-for-review.md`. In short: the template must be materially complete (not just section titles present), all status checks must be currently passing, and the only expected follow-up commits must be reviewer-driven. --> ## **Description** This PR upgrades to the final version of the money account upgrade controller - which can in theory run through the entire chomp upgrade flow. Things that we've changed in this PR in addition to bumping the package 1. Updated the config that is passed into the upgrade controller 2. Added a check which ensures the monad network is enabled before we start the upgrade process. This is necessary, because monad is enabled by default # TODO 1. ~~This PR builds on @MoMannn's PR MetaMask#29897 - and it should be merged first.~~ 2. This PR requires us to change the [vault config feature flag](https://app.launchdarkly.com/projects/metamask-client-config-api-mobile/flags/money-account-vault-config/targeting?env=test&selected-env=test) before it will function correctly. 3. Chomp returns a 500 on the final step of the upgrade process. We need to figure out why this is happening and fix it 4. ~~We need to merge and publish [this core pr](MetaMask/core#8621) and update the preview packages that are currently used in this branch to the real published updates.~~ <!-- Write a short description of the changes included in this pull request, also include relevant motivation and context. Have in mind the following questions: 1. What is the reason for the change? 2. What is the improvement/solution? --> ## **Changelog** <!-- If this PR is not End-User-Facing and should not show up in the CHANGELOG, you can choose to either: 1. Write `CHANGELOG entry: null` 2. Label with `no-changelog` If this PR is End-User-Facing, please write a short User-Facing description in the past tense like: `CHANGELOG entry: Added a new tab for users to see their NFTs` `CHANGELOG entry: Fixed a bug that was causing some NFTs to flicker` (This helps the Release Engineer do their job more quickly and accurately) --> CHANGELOG entry: Update to final version of money account upgrade controller ## **Related issues** Fixes: ## **Manual testing steps** ```gherkin Feature: my feature name Scenario: user [verb for user action] Given [describe expected initial app state] When user [verb for user action] Then [describe expected outcome] ``` ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** <!-- [screenshots/recordings] --> ### **After** <!-- [screenshots/recordings] --> ## **Pre-merge author checklist** <!-- Every checklist item must be consciously assessed before marking this PR as "Ready for review". A checked box means you deliberately considered that responsibility, not that you literally performed every action listed. Unchecked boxes are ambiguous: they are not an implicit "N/A" and they are not a silent "skip". See `docs/readme/ready-for-review.md` for the full checklist semantics. --> - [ ] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile 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 - [ ] I've documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] 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. #### Performance checks (if applicable) - [ ] I've tested on Android - Ideally on a mid-range device; emulator is acceptable - [ ] I've tested with a power user scenario - Use these [power-user SRPs](https://consensyssoftware.atlassian.net/wiki/spaces/TL1/pages/edit-v2/401401446401?draftShareId=9d77e1e1-4bdc-4be1-9ebb-ccd916988d93) to import wallets with many accounts and tokens - [ ] I've instrumented key operations with Sentry traces for production performance metrics - See [`trace()`](/app/util/trace.ts) for usage and [`addToken`](/app/components/Views/AddAsset/components/AddCustomToken/AddCustomToken.tsx#L274) for an example For performance guidelines and tooling, see the [Performance Guide](https://consensyssoftware.atlassian.net/wiki/spaces/TL1/pages/400085549067/Performance+Guide+for+Engineers). ## **Pre-merge reviewer checklist** <!-- Reviewer checklist items follow the same semantics as the author checklist: an unchecked box is ambiguous, a checked box means the reviewer consciously assessed that responsibility. See `docs/readme/ready-for-review.md`. --> - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] 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. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Touches money account upgrade bootstrap logic and programmatically adds missing networks via `NetworkController.addNetwork`, which can affect upgrade flow behavior and user network state if misconfigured. Also bumps to `@metamask/money-account-upgrade-controller@^2.0.0` with related dependency updates. > > **Overview** > Updates `moneyAccountUpgradeControllerInit` to initialize `MoneyAccountUpgradeController` using the Money Account *vault config* (chainId + boring vault address) instead of deriving addresses from CHOMP service details / delegator environment. > > Adds a bootstrap guard (`ensureChainConfigured`) that checks whether the vault chain is present in the user’s `NetworkController` config and, if missing, auto-adds it from `PopularList` (or logs an error and aborts if unsupported). Tests are updated to cover the new init parameters, missing vault config handling, and the chain auto-add behavior. > > Expands the controller messenger permissions for the full upgrade flow (delegation + additional CHOMP actions) and bumps `@metamask/money-account-upgrade-controller` to `^2.0.0` plus related dependency versions in `package.json`/`yarn.lock`. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 74a760e. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
Explanation
In this PR we
This has been tested in the mobile client - and it currently reaches the final step of the upgrade process where we
POST https://chomp.dev-api.cx.metamask.io/v1/intent- this currently returns a 500 error after a long delay. We're still investigating the cause of this, as there may be changes required to chomp.References
Checklist
Note
Medium Risk
Introduces new delegation-signing/storage and intent-registration steps plus breaking
init()and messenger-action requirements inMoneyAccountUpgradeController, which affects upgrade orchestration and on-chain/CHOMP interactions. Also changesChompApiServiceretry behavior for 4xx responses, which could alter client error handling and backoff characteristics.Overview
Completes the money account upgrade sequence by adding
build-delegationandregister-intentssteps after EIP-7702 authorization, including delegation creation/signing (viaDelegationController), CHOMP verification, persistence toAuthenticatedUserStorageService, and CHOMP intent registration (skipping existing active intents and re-registering revoked ones).Breaking API/config changes in
MoneyAccountUpgradeController:init()now takes{ chainId, boringVaultAddress }, resolves Delegation Framework contract/enforcer addresses from@metamask/delegation-deployments(throws if unsupported), and expands required messenger permissions to include storage, delegation signing, and CHOMP intent/delegation APIs.Retry semantics change in
@metamask/chomp-api-service: default retry policy now does not retry most 4xx responses (except429), while continuing to retry5xxand non-HTTP errors; tests updated/added to assert the new behavior and allow overriding viapolicyOptions.Reviewed by Cursor Bugbot for commit 0cf880a. Bugbot is set up for automated code reviews on this repo. Configure here.