fix(fees)!: canonical v0.6 ABI in harness/site/smoke; show the fee deposit in the snap - #45
Merged
Merged
Conversation
…posit in the snap - harness GenLayerFeeShim.sol, site prototype encoder, and smoke script were still encoding a stale pre-CON-504 protocol (8-field FeesDistribution with leaderTimeoutFee/validatorsTimeoutFee, bytes4 functionSelector allocation nodes) — transactions built by the E2E site could not even be parsed by the snap's own (correct) parser, so green wallet E2E proved nothing about v0.6. All three now encode the canonical shapes (10-field distribution incl. the three price caps; MessageFeeAllocationNode with messageType/onAcceptance/ callKey/bytes feeParams) matching the snap parser and genlayer-js - site fee math replaced with the consensus round-0 formula; units fixed (time-unit allocations are integer seconds, not parseUnits(x,18); realistic cap placeholders) - snap insight panel now shows the numbers that matter: Total / Fee deposit (= value − userValue) / userValue in GEN with raw wei secondary, and the bond being paid on appeal flows - integration test asserts a site-encoded transaction parses as fee-aware (fixture built by the SITE encoder, not the snap's own ABI constants) - snap.manifest.json shasum updated from an actual reproducible build
|
Warning Review limit reached
More reviews will be available in 25 minutes and 57 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughMigrate fee model to timeunit allocations and per-round execution budgeting; switch message allocation nodes to use callKey (bytes32) and encoded feeParams; update ABI/type bindings, encoders (RLP), UI forms/presets, smoke harness, Snap display/enrichment, and related tests. ChangesFee Schema and Core Implementation
MetaMask Snap Transaction Display and Enrichment
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/snap/src/components/TransactionConfig.tsx`:
- Around line 22-47: The parse of BigInt in formatWeiToGen can throw on
malformed input; wrap the BigInt(value) call in a try-catch inside
formatWeiToGen, catch any error from BigInt parsing, log or silently handle it
and return 'unknown' (same as undefined case) so the component doesn't throw;
keep the rest of the conversion logic unchanged and only proceed when BigInt
parsing succeeds.
- Around line 109-120: The fee deposit line in TransactionConfig is misleading:
change the parenthetical that reads "(+ userValue {formatWeiToGen(userValue)})"
to a clearer label such as "(userValue: {formatWeiToGen(userValue)})" or remove
the parenthetical entirely so it doesn't imply addition to feeDeposit; update
the JSX where feeDeposit and userValue are rendered (refer to variables
feeDeposit and userValue and the formatter formatWeiToGen) to reflect the chosen
wording and keep the existing Total display unchanged.
In `@packages/snap/src/index.tsx`:
- Around line 44-62: The BigInt conversion in withTransactionValue can throw if
summary.userValue is malformed; wrap the BigInt(summary.userValue) conversion in
a try-catch (inside withTransactionValue), and on error log or record the error
and fall back to 0n for userValue so processing continues safely; ensure you
still compute feeDeposit using the recovered userValue and return the same shape
(totalValue and feeDeposit as strings) from withTransactionValue.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 37fe8a59-b1af-4fa2-b400-970b16192a78
📒 Files selected for processing (10)
packages/harness/scripts/smoke.mjspackages/harness/src/GenLayerFeeShim.solpackages/site/src/pages/index.tsxpackages/site/src/prototype/transaction.tspackages/snap/snap.manifest.jsonpackages/snap/src/components/TransactionConfig.tsxpackages/snap/src/index.test.tsxpackages/snap/src/index.tsxpackages/snap/src/transactions/transaction.integration.test.tspackages/snap/src/transactions/transaction.ts
Comment on lines
+22
to
+47
| const formatWeiToGen = (value: string | undefined): string => { | ||
| if (value === undefined) { | ||
| return 'unknown'; | ||
| } | ||
|
|
||
| const wei = BigInt(value); | ||
| const whole = wei / WEI_PER_GEN; | ||
| const fraction = wei % WEI_PER_GEN; | ||
| if (fraction === 0n) { | ||
| return `${whole.toString()} GEN`; | ||
| } | ||
|
|
||
| const fractionText = fraction.toString().padStart(18, '0'); | ||
| const significantDigits = | ||
| whole > 0n | ||
| ? 6 | ||
| : Math.max( | ||
| 6, | ||
| fractionText.search(/[1-9]/u) + 6, | ||
| ); | ||
| const trimmedFraction = fractionText | ||
| .slice(0, Math.min(significantDigits, 18)) | ||
| .replace(/0+$/u, ''); | ||
|
|
||
| return `${whole.toString()}.${trimmedFraction} GEN`; | ||
| }; |
There was a problem hiding this comment.
Add defensive error handling for BigInt parsing.
While the upstream code should guarantee that value is a valid bigint string (from .toString() on bigints in withTransactionValue), the current implementation will throw an uncaught exception if an invalid string somehow reaches line 27. For defense in depth in a security-critical snap component, wrap the BigInt parsing in a try-catch block.
🛡️ Proposed fix to add error handling
const formatWeiToGen = (value: string | undefined): string => {
if (value === undefined) {
return 'unknown';
}
+ try {
const wei = BigInt(value);
const whole = wei / WEI_PER_GEN;
const fraction = wei % WEI_PER_GEN;
if (fraction === 0n) {
return `${whole.toString()} GEN`;
}
const fractionText = fraction.toString().padStart(18, '0');
const significantDigits =
whole > 0n
? 6
: Math.max(
6,
fractionText.search(/[1-9]/u) + 6,
);
const trimmedFraction = fractionText
.slice(0, Math.min(significantDigits, 18))
.replace(/0+$/u, '');
return `${whole.toString()}.${trimmedFraction} GEN`;
+ } catch {
+ return 'unknown';
+ }
};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/snap/src/components/TransactionConfig.tsx` around lines 22 - 47, The
parse of BigInt in formatWeiToGen can throw on malformed input; wrap the
BigInt(value) call in a try-catch inside formatWeiToGen, catch any error from
BigInt parsing, log or silently handle it and return 'unknown' (same as
undefined case) so the component doesn't throw; keep the rest of the conversion
logic unchanged and only proceed when BigInt parsing succeeds.
Comment on lines
+109
to
+120
| <Box direction="horizontal" alignment={'space-between'}> | ||
| <Text> | ||
| <Bold>Fee deposit:</Bold> | ||
| </Text> | ||
| <Text> | ||
| {formatWeiToGen(feeDeposit)} (+ userValue{' '} | ||
| {formatWeiToGen(userValue)}) | ||
| </Text> | ||
| </Box> | ||
| <Text> | ||
| <Italic>Raw fee deposit: {displayValue(feeDeposit)} wei</Italic> | ||
| </Text> |
There was a problem hiding this comment.
Clarify the fee deposit display text.
The text (+ userValue ...) on line 114 is misleading because it suggests that userValue is being added to feeDeposit, when in fact totalValue = feeDeposit + userValue (and Total is already displayed above). Consider revising the text to avoid confusion, such as (userValue: X GEN) or removing the parenthetical entirely since userValue is shown separately below.
✏️ Suggested clarification
<Text>
<Bold>Fee deposit:</Bold>
</Text>
<Text>
- {formatWeiToGen(feeDeposit)} (+ userValue{' '}
- {formatWeiToGen(userValue)})
+ {formatWeiToGen(feeDeposit)}
</Text>Or alternatively:
<Text>
- {formatWeiToGen(feeDeposit)} (+ userValue{' '}
- {formatWeiToGen(userValue)})
+ {formatWeiToGen(feeDeposit)} (userValue: {formatWeiToGen(userValue)})
</Text>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <Box direction="horizontal" alignment={'space-between'}> | |
| <Text> | |
| <Bold>Fee deposit:</Bold> | |
| </Text> | |
| <Text> | |
| {formatWeiToGen(feeDeposit)} (+ userValue{' '} | |
| {formatWeiToGen(userValue)}) | |
| </Text> | |
| </Box> | |
| <Text> | |
| <Italic>Raw fee deposit: {displayValue(feeDeposit)} wei</Italic> | |
| </Text> | |
| <Box direction="horizontal" alignment={'space-between'}> | |
| <Text> | |
| <Bold>Fee deposit:</Bold> | |
| </Text> | |
| <Text> | |
| {formatWeiToGen(feeDeposit)} (userValue: {formatWeiToGen(userValue)}) | |
| </Text> | |
| </Box> | |
| <Text> | |
| <Italic>Raw fee deposit: {displayValue(feeDeposit)} wei</Italic> | |
| </Text> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/snap/src/components/TransactionConfig.tsx` around lines 109 - 120,
The fee deposit line in TransactionConfig is misleading: change the
parenthetical that reads "(+ userValue {formatWeiToGen(userValue)})" to a
clearer label such as "(userValue: {formatWeiToGen(userValue)})" or remove the
parenthetical entirely so it doesn't imply addition to feeDeposit; update the
JSX where feeDeposit and userValue are rendered (refer to variables feeDeposit
and userValue and the formatter formatWeiToGen) to reflect the chosen wording
and keep the existing Total display unchanged.
Comment on lines
+44
to
+62
| const withTransactionValue = ( | ||
| summary: ParsedGenLayerTransaction, | ||
| value: unknown, | ||
| ): ParsedGenLayerTransaction => { | ||
| const totalValue = parseTransactionValue(value); | ||
| if (totalValue === undefined) { | ||
| return summary; | ||
| } | ||
|
|
||
| const userValue = | ||
| summary.userValue === undefined ? 0n : BigInt(summary.userValue); | ||
| const feeDeposit = totalValue > userValue ? totalValue - userValue : 0n; | ||
|
|
||
| return { | ||
| ...summary, | ||
| totalValue: totalValue.toString(), | ||
| feeDeposit: feeDeposit.toString(), | ||
| }; | ||
| }; |
There was a problem hiding this comment.
Add defensive error handling for BigInt conversion of userValue.
Line 54 calls BigInt(summary.userValue) without error handling. While summary.userValue should always be a valid bigint string from upstream parsing (via stringifyUint), adding a try-catch here provides defense in depth for a transaction handler in a security-critical snap.
🛡️ Proposed fix to add error handling
const withTransactionValue = (
summary: ParsedGenLayerTransaction,
value: unknown,
): ParsedGenLayerTransaction => {
const totalValue = parseTransactionValue(value);
if (totalValue === undefined) {
return summary;
}
+ try {
const userValue =
summary.userValue === undefined ? 0n : BigInt(summary.userValue);
const feeDeposit = totalValue > userValue ? totalValue - userValue : 0n;
return {
...summary,
totalValue: totalValue.toString(),
feeDeposit: feeDeposit.toString(),
};
+ } catch {
+ return {
+ ...summary,
+ totalValue: totalValue.toString(),
+ feeDeposit: totalValue.toString(),
+ };
+ }
};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/snap/src/index.tsx` around lines 44 - 62, The BigInt conversion in
withTransactionValue can throw if summary.userValue is malformed; wrap the
BigInt(summary.userValue) conversion in a try-catch (inside
withTransactionValue), and on error log or record the error and fall back to 0n
for userValue so processing continues safely; ensure you still compute
feeDeposit using the recovered userValue and return the same shape (totalValue
and feeDeposit as strings) from withTransactionValue.
- parseTransactionValue: validate with an explicit numeric pattern instead of a bare try/catch (the security scanner flags bare catch blocks as unhandled promise rejections; a regex guard is also more precise) - eslint/prettier fixes from --fix - snap.manifest.json shasum from rebuild
Local prettier disagrees with CI's pin on two sites; applied CI's exact suggested output. Snap manifest shasum from local rebuild — if CI's bundle still differs (build is environment-sensitive), the next commit adopts CI's computed value verbatim.
Push trigger was main-only; merges to v0.2-dev got no post-merge CI.
Local snap builds produce a different bundle hash than CI (environment- sensitive build — flagged for follow-up); CI is the gate, so its computed value is canonical.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
From the fee audit: the snap's parser was already canonical-correct, but the E2E scaffolding around it encoded a stale pre-CON-504 protocol — transactions built by the site couldn't be parsed by the snap's own fee-aware branch, so green wallet E2E proved nothing about v0.6.
GenLayerFeeShim.sol, site prototype encoder, and smoke script now encode the canonical shapes (10-field FeesDistribution incl. the three price caps;MessageFeeAllocationNodewith messageType/onAcceptance/callKey/bytes feeParams), byte-matching the snap parser and genlayer-js.parseUnits(x, 18); realistic cap placeholders instead of '110'/'150').snap.manifest.jsonshasum updated from an actual build.forge build, site+snap builds, lint, and snap test suite green.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Chores