Skip to content

Fix balance width - #831

Open
andrew-fleming wants to merge 9 commits into
OpenZeppelin:release/v0.3.0-alpha.1-rc.0from
andrew-fleming:fix-balance-width
Open

Fix balance width#831
andrew-fleming wants to merge 9 commits into
OpenZeppelin:release/v0.3.0-alpha.1-rc.0from
andrew-fleming:fix-balance-width

Conversation

@andrew-fleming

@andrew-fleming andrew-fleming commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Fixes #795

@andrew-fleming
andrew-fleming requested review from a team as code owners August 27, 2026 04:50
@coderabbitai

coderabbitai Bot commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: a0100448-e8b3-409a-82c3-eb6889282ed9

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Comment @coderabbitai help to get the list of available commands.

Comment thread contracts/src/token/test/ConfidentialFungibleToken.test.ts
Comment thread contracts/src/token/ConfidentialFungibleToken.compact
Comment thread contracts/src/token/test/ConfidentialFungibleToken.test.ts
Comment thread contracts/src/crypto/ElGamal.compact
Comment thread contracts/src/crypto/ElGamal.compact
Comment thread contracts/src/token/ConfidentialFungibleToken.compact Outdated

@0xisk 0xisk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! left some comments.

* @returns The wallet's cached plaintext for `ct`.
*/
witness wit_PlaintextBalance(ct: ElGamal_Ciphertext): Uint<128>;
witness wit_PlaintextBalance(ct: ElGamal_Ciphertext): Uint<248>;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: the doc eight lines up (line 342) still says the wallet caches a Map<ElGamal_Ciphertext, Uint128>, but the witness now returns Uint<248>.

added by claude (dev3-midnight-basic-review)

Comment on lines +1223 to +1226
const value = info.circuits
.find((c: { name: string }) => c.name === 'transfer')
.arguments.find((a: { name: string }) => a.name === 'value');
expect(BigInt(value.type.maxval)).toBe(MAX128);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this half reads transfer's value from the mock's own signature (MockConfidentialFungibleToken.compact:63 declares value: Uint<128>), so widening the module's transfer param would still pass here. The module bound is already pinned by keeps the transfer bound at the Uint<128> maximum above. Either drop it or say in a comment that it pins the mock surface.

added by claude (dev3-midnight-basic-review)

@0xisk

0xisk commented Sep 3, 2026

Copy link
Copy Markdown
Member

followup: ElGamal.assertDecryptsTo's claimedValue widens from Uint<128> to Uint<248>, changing a signature already released in 0.3.0-alpha, but CHANGELOG.md has no ### Changed entry under Unreleased. #762 on this branch carries one under ### Fixed.

(Commented here rather than inline since CHANGELOG.md isn't in the diff.)

added by claude (dev3-midnight-basic-review)

@0xisk 0xisk linked an issue Sep 3, 2026 that may be closed by this pull request
@0xisk

0xisk commented Sep 4, 2026

Copy link
Copy Markdown
Member

🔴 blocking: raising this from the followup above. ElGamal.assertDecryptsTo's claimedValue widens Uint<128>Uint<248>, changing a signature already published in 0.3.0-alpha, and CHANGELOG.md has no ### Changed entry under Unreleased. Shipping a release branch with an undocumented public-API change is the kind of gap the changelog exists to prevent, and it is a one-line fix. #762 on this branch already carries an entry under ### Fixed.

added by claude (dev3-midnight-basic-review)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

M-01: Missing Running Total Balance Check Can Lock Balances Permanently

2 participants