-
Notifications
You must be signed in to change notification settings - Fork 89
[RELEASE] fix batched USDC transactions #623
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 9 commits
61a97d7
86bf1b6
e31f040
8d11e86
1d67031
eac1e3c
b9799a1
ed9057d
c285dd1
1d854ae
c155f7d
00c50bb
51dedc6
d31535f
832a467
30ecdb3
1ebf7c9
7e95386
f425d59
379a75f
a2d0dde
35d234a
bc88aad
cf13b2e
64f00e7
00163ff
73d1c59
511c53b
c867824
1c233b4
141e755
fd5a3ea
e4d6c71
8b54e22
760e61d
023d0f5
1ccc6f3
0598857
ed2ac97
b82208a
bc7ce83
c9ce68b
b987d8d
ca91096
e7bb7c6
df698cb
358ddb5
9f9a58f
5d9dd8f
f945129
5213889
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -15,6 +15,12 @@ import { ProviderType } from 'providers/ProviderType'; | |||||||||||||||||||||||||
| import { Tool } from 'openai/resources/responses/responses'; | ||||||||||||||||||||||||||
| import { SupportedVideoModel } from '@merit-systems/echo-typescript-sdk'; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| export function applyMaxCostMarkup(maxCost: Decimal): Decimal { | ||||||||||||||||||||||||||
| const markup = process.env.MAX_COST_MARKUP || '1.25'; | ||||||||||||||||||||||||||
| return maxCost.mul(new Decimal(markup)); | ||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
The View DetailsAnalysisInvalid MAX_COST_MARKUP environment variable crashes serverWhat fails: How to reproduce: # Set invalid environment variable and make a request:
MAX_COST_MARKUP="invalid" node -e "
const { applyMaxCostMarkup } = require('./dist/server.js');
const { Decimal } = require('@prisma/client/runtime/library');
applyMaxCostMarkup(new Decimal('100'));
"Result: Expected: Application should handle invalid environment variables gracefully and continue serving requests with fallback values. Fix: Added try-catch validation in |
||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| export function getRequestMaxCost( | ||||||||||||||||||||||||||
| req: EscrowRequest, | ||||||||||||||||||||||||||
| provider: BaseProvider, | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,12 @@ | ||
| import { encodeFunctionData, Abi } from 'viem'; | ||
| import { encodeFunctionData, Abi, formatUnits, parseUnits } from 'viem'; | ||
| import { | ||
| MERIT_ABI, | ||
| MERIT_CONTRACT_ADDRESS, | ||
| USDC_ADDRESS, | ||
| ERC20_CONTRACT_ABI, | ||
| ETH_ADDRESS, | ||
| } from './constants'; | ||
| import logger from '../../logger'; | ||
| import logger, { logMetric } from '../../logger'; | ||
| import { getSmartAccount } from 'utils'; | ||
|
|
||
| export interface FundRepoResult { | ||
|
|
@@ -106,3 +107,73 @@ export async function safeFundRepo(amount: number): Promise<void> { | |
| ); | ||
| } | ||
| } | ||
|
|
||
|
|
||
| export async function safeFundRepoIfWorthwhile(): Promise<void> { | ||
| const repoId = process.env.MERIT_REPO_ID; | ||
| if (!repoId) { | ||
| throw new Error('Missing required environment variables'); | ||
| } | ||
|
|
||
| // check balance of wallet. If it is > 100 USD, send all of the USD to the repo. | ||
|
|
||
| const { smartAccount } = await getSmartAccount(); | ||
| const balances = await smartAccount.listTokenBalances({ | ||
| network: 'base', | ||
| }); | ||
| const baseUsdcBalance = balances.balances.find((balance) => balance.token.contractAddress === USDC_ADDRESS); | ||
|
|
||
|
|
||
| const ethereumBalance = balances.balances.find((balance) => balance.token.contractAddress === ETH_ADDRESS); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The View Details📝 Patch Detailsdiff --git a/packages/app/server/src/services/fund-repo/fundRepoService.ts b/packages/app/server/src/services/fund-repo/fundRepoService.ts
index b3d09349..520d210d 100644
--- a/packages/app/server/src/services/fund-repo/fundRepoService.ts
+++ b/packages/app/server/src/services/fund-repo/fundRepoService.ts
@@ -116,6 +116,10 @@ export async function safeFundRepoIfWorthwhile(): Promise<void> {
throw new Error('Missing required environment variables');
}
+ if (!ETH_ADDRESS) {
+ throw new Error('Missing required environment variables');
+ }
+
// check balance of wallet. If it is > 100 USD, send all of the USD to the repo.
const { smartAccount } = await getSmartAccount();
AnalysisMissing ETH_ADDRESS validation causes silent funding failures in safeFundRepoIfWorthwhile()What fails: How to reproduce: # Start application without ETH_ADDRESS set
unset ETH_ADDRESS
node src/server.ts
# Call safeFundRepoIfWorthwhile() with wallet having >0.0001 ETH and >100 USDCResult: Function logs "No Ethereum balance found, skipping fundRepo event" and returns early (lines 130-132), preventing legitimate repo funding. The Expected: Should throw "Missing required environment variables" error like other undefined env vars ( References: Environment variable validation best practices recommend fail-fast validation over silent failures in business logic. |
||
|
|
||
| if (!ethereumBalance) { | ||
| logger.info('No Ethereum balance found, skipping fundRepo event'); | ||
| return; | ||
| } | ||
|
|
||
| if (!baseUsdcBalance) { | ||
| logger.info('No base USDC balance found, skipping fundRepo event'); | ||
| return; | ||
| } | ||
|
|
||
| const ethereumBalanceAmount = ethereumBalance.amount.amount; | ||
| const ethBalanceFormatted = formatUnits(ethereumBalanceAmount, ethereumBalance.amount.decimals); | ||
| logger.info(`Ethereum balance is ${ethBalanceFormatted} ETH`, { | ||
| amount: ethBalanceFormatted, | ||
| address: smartAccount.address, | ||
| }); | ||
|
|
||
| const baseUsdcBalanceAmount = baseUsdcBalance.amount.amount; | ||
| const usdcBalanceFormatted = formatUnits(baseUsdcBalanceAmount, baseUsdcBalance.amount.decimals); | ||
| logger.info(`Base USDC balance is ${usdcBalanceFormatted} USD`, { | ||
| amount: usdcBalanceFormatted, | ||
| address: smartAccount.address, | ||
| }); | ||
|
|
||
| const ETH_WARNING_THRESHOLD = parseUnits( | ||
| String(process.env.ETH_WARNING_THRESHOLD || '0.0001'), | ||
| ethereumBalance.amount.decimals | ||
| ); | ||
| const BASE_USDC_WARNING_THRESHOLD = parseUnits( | ||
| String(process.env.BASE_USDC_TRANSFER_THRESHOLD || '5'), | ||
| baseUsdcBalance.amount.decimals | ||
| ); | ||
|
|
||
| if (ethereumBalanceAmount < ETH_WARNING_THRESHOLD) { | ||
| const readableEthWarningThreshold = formatUnits(ETH_WARNING_THRESHOLD, ethereumBalance.amount.decimals); | ||
| logger.error(`[Critical] Ethereum balance is less than ${readableEthWarningThreshold} ETH, skipping fundRepo event`); | ||
| logMetric('fund_repo.ethereum_balance_running_low', 1, { | ||
| amount: ethBalanceFormatted, | ||
| address: smartAccount.address, | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| if (baseUsdcBalanceAmount < BASE_USDC_WARNING_THRESHOLD) { | ||
| logger.info('Base USDC balance is less than threshold, skipping fundRepo event'); | ||
| return; | ||
| } | ||
| logger.info(`Base USDC balance is ${usdcBalanceFormatted} USD, funding repo`); | ||
|
|
||
| await safeFundRepo(Number(usdcBalanceFormatted)); | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The refund calculation incorrectly applies markup to the actual transaction cost, causing users to be overcharged when the actual cost is lower than the estimated maximum cost.
View Details
Analysis
Double markup application in finalize() causes user overcharges
What fails:
finalize()function in/packages/app/server/src/handlers.tsapplies markup twice - once to estimate (server.ts:111) then again to actual cost (handlers.ts:121), causing users to pay more than actual transaction cost + intended markupHow to reproduce:
Result: Users systematically overcharged when actual costs are below estimates. In test case: 12.5 USD overcharge (25% of actual cost).
Expected: Refund should be
paymentAmount - rawTransactionCostso users pay exactly the actual cost, with markup profit tracked separately for fundRepo operations.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is actually correct. The user should be refunded paymentAmountDecimal - transactionCostWithMarkup.
The final cost to the user should be raw tx cost + markup amount.