Feat/shield create subscription crypto by tuna1207 · Pull Request #6456 · MetaMask/core

tuna1207 · 2025-09-04T06:14:54Z

Explanation

Add getCryptoApproveTransactionParams which handle validating and prepare params to create crypto approve transaction for user to sign then submit to subscription service
Add startSubscriptionWithCrypto which register submitted crypto transaction with subscription service to start subscription

References

Checklist

I've updated the test suite for new or updated code as appropriate
I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
I've communicated my changes to consumers by updating changelogs for packages I've changed, highlighting breaking changes as necessary
I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes

…aciton in controller

matthiasgeihs · 2025-09-08T08:55:33Z

+    // conversion rate is a float string e.g: "1.0"
+    // We need to handle float conversion rates with integer math for BigInt.
+    // We'll scale the conversion rate to an integer by multiplying by 10^4.
+    // conversionRate is in usd decimal. In most currencies, we only care about 2 decimals (cents)
+    // So, scale must be max of 10 ** 4 (most exchanges trade with max 4 decimals of usd)
+    // This allows us to avoid floating point math and keep precision.
+    const CONVERSION_RATE_SCALE = 10n ** 4n;
+    const conversionRateScaled = BigInt(
+      Number(conversionRate) * Number(CONVERSION_RATE_SCALE),
+    );
+    const priceAmount = this.#getSubscriptionPriceAmount(price);
+    const priceAmountScaled = BigInt(
+      priceAmount * Number(CONVERSION_RATE_SCALE),
+    );
+
+    const amount =
+      ((priceAmountScaled / CONVERSION_RATE_SCALE) *
+        BigInt(10) ** BigInt(tokenPaymentInfo.decimals) *
+        CONVERSION_RATE_SCALE) /
+      conversionRateScaled;


I think we should be able to do that computation without relying on floating point arithmetic.

Does the following work?

tokenApproveAmount = priceInUSD * tokenPerUSD = (priceAmount / priceDecimals) * (tokenAmount / tokenDecimals) = (priceAmount * tokenAmount) / (priceDecimals * tokenDecimals)

Co-authored-by: matthiasgeihs <62935430+matthiasgeihs@users.noreply.github.com>

cursor · 2025-09-09T08:34:21Z

+        BigInt(10) ** BigInt(tokenPaymentInfo.decimals) *
+        CONVERSION_RATE_SCALE) /
+      conversionRateScaled;
+    return amount;


Bug: Token Approval Bug Causes Precision Loss

The token approve amount calculation in #getTokenApproveAmount has a precision loss bug. The BigInt integer division (priceAmountScaled / CONVERSION_RATE_SCALE) truncates the result, especially for small or fractional subscription prices. This can lead to an incorrect or zero token approval amount, causing transaction failures.

matthiasgeihs · 2025-09-09T09:48:00Z

@@ -1,5 +1,4 @@
 import { handleFetch } from '@metamask/controller-utils';
-import nock, { cleanAll, isDone } from 'nock';


Why did you remove the usage of nock here?
Remember that we should keep changes minimal within the context of the PR, if possible.

matthiasgeihs · 2025-09-09T09:50:53Z

@@ -1,3 +1,5 @@
+import { handleFetch } from '@metamask/controller-utils';


need to move @metamask/controller-utils to main dependencies now

cursor · 2025-09-09T10:08:48Z

+    const tokenAmount =
+      (priceAmountScaled * tokenDecimal) / conversionRateScaled;
+    return tokenAmount;
+  }


Bug: Token Amount Calculation Precision Error

The #getTokenApproveAmount method's BigInt scaling logic immediately divides scaled values by SCALE using integer division. This truncates decimal precision, leading to incorrect token amount calculations for crypto subscriptions.

…eld-create-subscription-crypto

cursor · 2025-09-09T10:31:29Z

+    const tokenAmount =
+      (priceAmountScaled * tokenDecimal) / conversionRateScaled;
+    return tokenAmount;
+  }


Bug: BigInt Scaling Logic Fails

The BigInt scaling logic in #getTokenApproveAmount loses precision. Values are scaled up by SCALE and then immediately divided by SCALE using integer division, which truncates decimal values. This defeats the purpose of scaling, resulting in incorrect conversionRateScaled and priceAmountScaled values, and ultimately wrong token approval amounts for crypto payments.

as far as i can see comments have been addressed.
dismissing so we can move forward with other features.
(if issues persist, we still have some time to address them before we release the first version of the controller.)

cursor · 2025-09-09T10:44:47Z

+
+    const tokenAmount =
+      (priceAmountScaled * tokenDecimal) / conversionRateScaled;
+    return tokenAmount;


Bug: Token Approval Precision Loss

The scaling logic in #getTokenApproveAmount is flawed. conversionRateScaled and priceAmountScaled are scaled up by SCALE then immediately divided by SCALE using BigInt integer division. This truncates fractional parts, causing precision loss and incorrect token approval amounts. It also risks division by zero if conversionRateScaled becomes 0n.

tuna1207 added 9 commits September 1, 2025 16:07

feat: add get price info method

c74dd4d

feat: update fetch fn

bc86539

feat: handle create crypto approve

55e415d

Merge branch 'main' into feat/shield-create-subscription-crypto

ef91e6c

refactor: simplify test mock data

ceb20ac

feat: register createCryptoApproveTransaction handler

a5903c4

feat: add start crypto subscription method

b8bd772

feat: register start crypto subscription handler

1141428

feat: correct type name and export

8d8f0c8

tuna1207 requested review from chaitanyapotti and matthiasgeihs September 4, 2025 06:14

tuna1207 added 8 commits September 4, 2025 14:46

chore: update changelog

ede07ec

fix: tsconfig

f938bb2

fix: correct tsconfig

e49aed4

chore: upgrade transaction-controller dependencies version

fad2fea

Merge branch 'main' into feat/shield-create-subscription-crypto

15afd77

feat: use getCryptoApproveTransactionParams instead of creating trans…

2634ca8

…aciton in controller

feat: correct package usage

c148d80

fix: linting

3e13fe7

chaitanyapotti reviewed Sep 6, 2025

View reviewed changes

Comment thread packages/subscription-controller/src/SubscriptionController.ts Outdated

chaitanyapotti reviewed Sep 6, 2025

View reviewed changes

Comment thread packages/subscription-controller/src/SubscriptionController.ts Outdated

fix: update conversion rate scale

18ac5a4

tuna1207 marked this pull request as ready for review September 6, 2025 07:05

tuna1207 requested review from a team as code owners September 6, 2025 07:05

This comment was marked as outdated.

Sign in to view

feat: scale subscription price amount

247e2ac

This comment was marked as outdated.

Sign in to view

chaitanyapotti previously approved these changes Sep 8, 2025

View reviewed changes

chaitanyapotti enabled auto-merge (squash) September 8, 2025 04:54

tuna1207 dismissed chaitanyapotti’s stale review via def4585 September 8, 2025 09:20

tuna1207 requested a review from himanshuchawla009 September 8, 2025 09:20

matthiasgeihs requested changes Sep 8, 2025

View reviewed changes

refactor: rename response type for consistency

8184712

This comment was marked as outdated.

Sign in to view

Update packages/subscription-controller/CHANGELOG.md

dee0f45

Co-authored-by: matthiasgeihs <62935430+matthiasgeihs@users.noreply.github.com>

This comment was marked as outdated.

Sign in to view

feat: use handleFetch internally

b6f696b

This comment was marked as outdated.

Sign in to view

fix: remove redundant test

048f5c5

This comment was marked as outdated.

Sign in to view

refactor: revert moving

d4fa47e

cursor Bot reviewed Sep 9, 2025

View reviewed changes

chaitanyapotti removed the request for review from lwin-kyaw September 9, 2025 09:09

matthiasgeihs requested changes Sep 9, 2025

View reviewed changes

refactor: make scale more clear

3f0df4b

cursor Bot reviewed Sep 9, 2025

View reviewed changes

tuna1207 added 2 commits September 9, 2025 17:26

chore: move package to dep

3ae5b3f

Merge commit '7a9e620105aadd8902abe8c7976f57e6e88bf867' into feat/shi…

c122653

…eld-create-subscription-crypto

matthiasgeihs previously approved these changes Sep 9, 2025

View reviewed changes

cursor Bot reviewed Sep 9, 2025

View reviewed changes

fix: lint

6853156

tuna1207 dismissed matthiasgeihs’s stale review via 6853156 September 9, 2025 10:41

matthiasgeihs approved these changes Sep 9, 2025

View reviewed changes

cursor Bot reviewed Sep 9, 2025

View reviewed changes

chaitanyapotti merged commit e09ffed into main Sep 9, 2025
239 checks passed

chaitanyapotti deleted the feat/shield-create-subscription-crypto branch September 9, 2025 10:47

		@@ -1,5 +1,4 @@
		import { handleFetch } from '@metamask/controller-utils';
		import nock, { cleanAll, isDone } from 'nock';

		@@ -1,3 +1,5 @@
		import { handleFetch } from '@metamask/controller-utils';

Uh oh!

Conversation

tuna1207 commented Sep 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Explanation

References

Checklist

Uh oh!

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

matthiasgeihs Sep 8, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

cursor Bot Sep 9, 2025

Choose a reason for hiding this comment

Bug: Token Approval Bug Causes Precision Loss

Uh oh!

matthiasgeihs Sep 9, 2025

Choose a reason for hiding this comment

Uh oh!

matthiasgeihs Sep 9, 2025

Choose a reason for hiding this comment

Uh oh!

cursor Bot Sep 9, 2025

Choose a reason for hiding this comment

Bug: Token Amount Calculation Precision Error

Uh oh!

cursor Bot Sep 9, 2025

Choose a reason for hiding this comment

Bug: BigInt Scaling Logic Fails

Uh oh!

cursor Bot Sep 9, 2025

Choose a reason for hiding this comment

Bug: Token Approval Precision Loss

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tuna1207 commented Sep 4, 2025 •

edited

Loading