Add tryAdd and trySub to FHESafeMath
#206
Merged
Conversation
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
Contributor
Reviewer's GuideThis PR introduces new safe math functions tryAdd and trySub with overflow/underflow checks into FHESafeMath, exposes them in the mock contract, adds comprehensive unit tests covering various cases, and includes a changeset for tracking. Sequence diagram for tryAdd interaction in FHESafeMathMocksequenceDiagram
participant User
participant FHESafeMathMock
participant FHESafeMath
participant FHE
User->>FHESafeMathMock: tryAdd(a, b)
FHESafeMathMock->>FHESafeMath: tryAdd(a, b)
FHESafeMath->>FHE: isInitialized(a)
FHESafeMath->>FHE: isInitialized(b)
FHESafeMath->>FHE: add(a, b)
FHESafeMath->>FHE: ge(sum, a)
FHESafeMath->>FHE: select(success, sum, 0)
FHESafeMath-->>FHESafeMathMock: (success, sum)
FHESafeMathMock->>FHE: allowThis(success)
FHESafeMathMock->>FHE: allow(success, User)
FHESafeMathMock->>FHE: isInitialized(sum)
FHESafeMathMock->>FHE: allowThis(sum)
FHESafeMathMock->>FHE: allow(sum, User)
FHESafeMathMock-->>User: emit ResultComputed(success, sum)
Sequence diagram for trySub interaction in FHESafeMathMocksequenceDiagram
participant User
participant FHESafeMathMock
participant FHESafeMath
participant FHE
User->>FHESafeMathMock: trySub(a, b)
FHESafeMathMock->>FHESafeMath: trySub(a, b)
FHESafeMath->>FHE: isInitialized(a)
FHESafeMath->>FHE: isInitialized(b)
FHESafeMath->>FHE: sub(a, b)
FHESafeMath->>FHE: le(difference, a)
FHESafeMath->>FHE: select(success, difference, 0)
FHESafeMath-->>FHESafeMathMock: (success, difference)
FHESafeMathMock->>FHE: allowThis(success)
FHESafeMathMock->>FHE: allow(success, User)
FHESafeMathMock->>FHE: isInitialized(difference)
FHESafeMathMock->>FHE: allowThis(difference)
FHESafeMathMock->>FHE: allow(difference, User)
FHESafeMathMock-->>User: emit ResultComputed(success, difference)
Class diagram for updated FHESafeMath libraryclassDiagram
class FHESafeMath {
+tryAdd(euint64 a, euint64 b) returns (ebool success, euint64 res)
+trySub(euint64 a, euint64 b) returns (ebool success, euint64 res)
}
Class diagram for updated FHESafeMathMock contractclassDiagram
class FHESafeMathMock {
+tryAdd(euint64 a, euint64 b) returns (ebool success, euint64 sum)
+trySub(euint64 a, euint64 b) returns (ebool success, euint64 difference)
}
FHESafeMathMock --> FHESafeMath : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds internal overflow-aware addition and subtraction helpers to FHESafeMath (tryAdd, trySub), exposes them via public mock wrappers, updates tests to cover these operations across varied inputs, and includes a changeset for a minor release. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Tester
participant Mock as FHESafeMathMock
participant Lib as FHESafeMath
Note over Lib: New internal helpers: tryAdd / trySub
Tester->>Mock: tryAdd(a,b) / trySub(a,b)
Mock->>Lib: tryAdd(a,b) / trySub(a,b)
alt any input uninitialized
Lib-->>Mock: (success=true, res=pass-through input)
else compute
Lib-->>Lib: perform FHE.add / FHE.sub
alt no overflow/underflow
Lib-->>Mock: (success=true, res=computed)
else overflow/underflow
Lib-->>Mock: (success=false, res=0)
end
end
Mock-->>Tester: emit ResultComputed(success, res) and return (success, res)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Comment |
✅ Deploy Preview for confidential-tokens ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Review trySub's initialization check for single uninitialized operands to match tryAdd's logic and avoid inconsistent behavior.
- Refactor the duplicated FHE.allowThis/FHE.allow blocks in FHESafeMathMock into a helper utility to reduce boilerplate.
- Add a subtraction underflow test case for large values (e.g., a=0, b=MaxUint64) to ensure full boundary coverage.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Review trySub's initialization check for single uninitialized operands to match tryAdd's logic and avoid inconsistent behavior.
- Refactor the duplicated FHE.allowThis/FHE.allow blocks in FHESafeMathMock into a helper utility to reduce boilerplate.
- Add a subtraction underflow test case for large values (e.g., a=0, b=MaxUint64) to ensure full boundary coverage.
## Individual Comments
### Comment 1
<location> `contracts/utils/FHESafeMath.sol:55` </location>
<code_context>
+ }
+
+ euint64 sum = FHE.add(a, b);
+ success = FHE.ge(sum, a);
+ res = FHE.select(success, sum, FHE.asEuint64(0));
+ }
+
</code_context>
<issue_to_address>
Overflow detection in tryAdd may not be robust for all FHE implementations.
The current overflow check relies on FHE.ge(sum, a), which may not be reliable across all FHE libraries. Please verify the arithmetic semantics and consider implementing more robust overflow detection if necessary.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
euint64 sum = FHE.add(a, b);
success = FHE.ge(sum, a);
res = FHE.select(success, sum, FHE.asEuint64(0));
}
=======
euint64 sum = FHE.add(a, b);
// Overflow occurs if sum < a or sum < b for unsigned addition
ebool overflowA = FHE.lt(sum, a);
ebool overflowB = FHE.lt(sum, b);
ebool overflow = FHE.or(overflowA, overflowB);
success = FHE.not(overflow);
res = FHE.select(success, sum, FHE.asEuint64(0));
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `contracts/utils/FHESafeMath.sol:69` </location>
<code_context>
+ }
+
+ euint64 difference = FHE.sub(a, b);
+ success = FHE.le(difference, a);
+ res = FHE.select(success, difference, FHE.asEuint64(0));
+ }
}
</code_context>
<issue_to_address>
Underflow detection in trySub may not cover all edge cases.
If FHE.sub wraps on underflow, FHE.le(difference, a) may not reliably detect it. Please verify whether FHE guarantees safe subtraction or add an explicit underflow check.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
euint64 difference = FHE.sub(a, b);
success = FHE.le(difference, a);
res = FHE.select(success, difference, FHE.asEuint64(0));
=======
ebool canSubtract = FHE.ge(a, b);
euint64 difference = FHE.sub(a, b);
success = canSubtract;
res = FHE.select(canSubtract, difference, FHE.asEuint64(0));
>>>>>>> REPLACE
</suggested_fix>
### Comment 3
<location> `test/utils/FHESafeMath.test.ts:96` </location>
<code_context>
}
});
+
+ const addArgsOptions: [BigNumberish | undefined, BigNumberish | undefined, BigNumberish | undefined, boolean][] = [
+ // a + b = c & success
+ [undefined, undefined, undefined, true],
+ [undefined, 0, 0, true],
+ [0, undefined, 0, true],
+ [1, 1, 2, true],
+ [undefined, 1, 1, true],
+ [0, 1, 1, true],
+ [0, MaxUint64, MaxUint64, true],
+ [1, MaxUint64, 0, false],
+ ];
+ const subArgsOptions: [number | undefined, number | undefined, number | undefined, boolean][] = [
</code_context>
<issue_to_address>
Missing test case for addition with both operands at MaxUint64.
Please add a test for tryAdd with both operands set to MaxUint64 to verify correct overflow handling.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
const addArgsOptions: [BigNumberish | undefined, BigNumberish | undefined, BigNumberish | undefined, boolean][] = [
// a + b = c & success
[undefined, undefined, undefined, true],
[undefined, 0, 0, true],
[0, undefined, 0, true],
[1, 1, 2, true],
[undefined, 1, 1, true],
[0, 1, 1, true],
[0, MaxUint64, MaxUint64, true],
[1, MaxUint64, 0, false],
];
=======
const addArgsOptions: [BigNumberish | undefined, BigNumberish | undefined, BigNumberish | undefined, boolean][] = [
// a + b = c & success
[undefined, undefined, undefined, true],
[undefined, 0, 0, true],
[0, undefined, 0, true],
[1, 1, 2, true],
[undefined, 1, 1, true],
[0, 1, 1, true],
[0, MaxUint64, MaxUint64, true],
[1, MaxUint64, 0, false],
[MaxUint64, MaxUint64, 0, false], // Added test case for both operands at MaxUint64
];
>>>>>>> REPLACE
</suggested_fix>
### Comment 4
<location> `test/utils/FHESafeMath.test.ts:107` </location>
<code_context>
+ [0, MaxUint64, MaxUint64, true],
+ [1, MaxUint64, 0, false],
+ ];
+ const subArgsOptions: [number | undefined, number | undefined, number | undefined, boolean][] = [
+ // a - b = c & success
+ [undefined, undefined, undefined, true],
+ [undefined, 0, 0, true],
+ [0, undefined, 0, true],
+ [1, 1, 0, true],
+ [undefined, 1, 0, false],
+ [0, 1, 0, false],
+ ];
+
</code_context>
<issue_to_address>
Missing test case for subtraction with both operands undefined.
Please specify the intended outcome for trySub when both operands are undefined, and add a test case to verify the result and success flag if this scenario is supported.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
.changeset/public-dolls-lose.md (1)
5-5: Clarify behavior difference vs. tryIncrease/tryDecrease.State explicitly that, unlike tryIncrease/tryDecrease (which return the original value on failure), tryAdd/trySub return 0 on failure to avoid confusion for integrators.
-`FHESafeMath`: Add `tryAdd` and `trySub` functions that return 0 upon failure. +`FHESafeMath`: Add `tryAdd` and `trySub` functions that return 0 upon failure (note: unlike `tryIncrease`/`tryDecrease`, which return the original value on failure).contracts/utils/FHESafeMath.sol (1)
59-62: Nit: wording — “subtract b from a”.The docstring should read “subtract
bfroma” for clarity.- * @dev Try to subtract `a` and `b`. If the operation is successful, `success` will be true and `res` + * @dev Try to subtract `b` from `a`. If the operation is successful, `success` will be true and `res`test/utils/FHESafeMath.test.ts (2)
96-107: Add symmetric overflow case for add (MaxUint64 + 1).To mirror
[1, MaxUint64], include[MaxUint64, 1]to catch operand-order regressions.const addArgsOptions: [BigNumberish | undefined, BigNumberish | undefined, BigNumberish | undefined, boolean][] = [ // a + b = c & success [undefined, undefined, undefined, true], [undefined, 0, 0, true], [0, undefined, 0, true], [1, 1, 2, true], [undefined, 1, 1, true], [0, 1, 1, true], [0, MaxUint64, MaxUint64, true], + [MaxUint64, 1, 0, false], [1, MaxUint64, 0, false], ];
107-116: Cover the b-uninitialized path for subtraction with non-zero a.Add
[1, undefined, 1, true]to ensurea - undefinedbehaves likea - 0.const subArgsOptions: [number | undefined, number | undefined, number | undefined, boolean][] = [ // a - b = c & success [undefined, undefined, undefined, true], [undefined, 0, 0, true], [0, undefined, 0, true], + [1, undefined, 1, true], [1, 1, 0, true], [undefined, 1, 0, false], [0, 1, 0, false], ];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.changeset/public-dolls-lose.md(1 hunks)contracts/mocks/utils/FHESafeMathMock.sol(1 hunks)contracts/utils/FHESafeMath.sol(1 hunks)test/utils/FHESafeMath.test.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/utils/FHESafeMath.test.ts (1)
test/helpers/event.ts (1)
callAndGetResult(5-9)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (5)
contracts/utils/FHESafeMath.sol (1)
42-57: Overflow check and uninitialized handling in tryAdd look correct.Branching on uninitialized operands and detecting overflow via
sum >= ais sound foreuint64. Returning 0 on failure matches the changeset.contracts/mocks/utils/FHESafeMathMock.sol (2)
45-56: Wrapper mirrors allow-listing and event pattern correctly.Public
tryAddwrapper properly delegates, exposessuccess, and conditionally grants access to the result.
58-69: Wrapper mirrors allow-listing and event pattern correctly.Public
trySubwrapper follows the established pattern and will work with the proposedtrySubguards.test/utils/FHESafeMath.test.ts (2)
6-6: LGTM: BigNumberish import is appropriate for MaxUint64 cases.
117-158: Parametric harness for tryAdd/trySub looks solid.Dynamic dispatch via full function signature and event-based result extraction are correct and consistent with mocks.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
test/utils/FHESafeMath.test.ts (4)
96-107: Add symmetry and edge cases for tryAdd.Cover both operand order and near-boundary transitions to harden overflow checks.
Apply:
const addArgsOptions: [BigNumberish | undefined, BigNumberish | undefined, BigNumberish | undefined, boolean][] = [ // a + b = c & success [undefined, undefined, undefined, true], [undefined, 0, 0, true], [0, undefined, 0, true], [1, 1, 2, true], [undefined, 1, 1, true], [0, 1, 1, true], [0, MaxUint64, MaxUint64, true], + [MaxUint64, 0n, MaxUint64, true], + [undefined, MaxUint64, MaxUint64, true], + [MaxUint64 - 1n, 1n, MaxUint64, true], + [MaxUint64 - 1n, 2n, 0n, false], [1, MaxUint64, 0, false], [MaxUint64, MaxUint64, 0, false], ];
108-116: Unify types to BigNumberish and expand trySub boundaries.Keep parity with add-cases and exercise 64-bit edges.
Apply:
- const subArgsOptions: [number | undefined, number | undefined, number | undefined, boolean][] = [ + const subArgsOptions: [BigNumberish | undefined, BigNumberish | undefined, BigNumberish | undefined, boolean][] = [ // a - b = c & success [undefined, undefined, undefined, true], - [undefined, 0, 0, true], - [0, undefined, 0, true], - [1, 1, 0, true], - [undefined, 1, 0, false], - [0, 1, 0, false], + [undefined, 0, 0, true], + [0, undefined, 0, true], + [1, 1, 0, true], + [MaxUint64, MaxUint64, 0, true], + [MaxUint64, 1n, MaxUint64 - 1n, true], + [0, MaxUint64, 0, false], + [1, MaxUint64, 0, false], + [undefined, 1, 0, false], ];
130-131: Nit: Use equality for suite label.Avoid accidental matches if other names appear; equality is clearer.
- describe(`try ${params.functionSignature.startsWith('tryAdd') ? 'add' : 'sub'}`, function () { + describe(`try ${params.functionSignature === 'tryAdd(bytes32,bytes32)' ? 'add' : 'sub'}`, function () {
144-147: Avoid any and string-indexing: pick the typed function once.Prevents typos in the signature and keeps TypeChain type-safety.
- const [success, updated] = await callAndGetResult( - (fheSafeMath as any)[params.functionSignature](handleA, handleB), - resultComputedSignature, - ); + const fn = + params.functionSignature === 'tryAdd(bytes32,bytes32)' + ? fheSafeMath['tryAdd(bytes32,bytes32)'] + : fheSafeMath['trySub(bytes32,bytes32)']; + const [success, updated] = await callAndGetResult(fn(handleA, handleB), resultComputedSignature);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/utils/FHESafeMath.test.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/utils/FHESafeMath.test.ts (1)
test/helpers/event.ts (1)
callAndGetResult(5-9)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: boostsecurity - boostsecurityio/semgrep-pro
- GitHub Check: slither
- GitHub Check: tests
- GitHub Check: coverage
🔇 Additional comments (1)
test/utils/FHESafeMath.test.ts (1)
6-6: LGTM: Stronger typing with BigNumberish.Importing BigNumberish is appropriate for 64-bit boundary values (e.g., MaxUint64) used in new cases.
james-toussaint
previously approved these changes
Sep 15, 2025
| [MaxUint64, MaxUint64, 0, true], | ||
| ]; | ||
|
|
||
| for (const params of [ |
Contributor
There was a problem hiding this comment.
Maybe be can merge tryIncrease/tryDecrease into this.
Co-authored-by: James Toussaint <33313130+james-toussaint@users.noreply.github.com>
Co-authored-by: James Toussaint <33313130+james-toussaint@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary by Sourcery
Introduce tryAdd and trySub methods in FHESafeMath for bounded addition and subtraction, update the mock contract to support them, and extend the test suite to validate their behavior under various success and failure cases.
New Features:
Enhancements:
Tests:
Chores:
Summary by CodeRabbit
New Features
Tests