Votes Confidential #40

arr00 · 2025-05-19T15:28:35Z

No description provided.

netlify · 2025-06-05T21:30:00Z

✅ Deploy Preview for confidential-tokens ready!

Name	Link
🔨 Latest commit	`1aa2eec`
🔍 Latest deploy log	https://app.netlify.com/projects/confidential-tokens/deploys/6866d5f64b8ede00082afccf
😎 Deploy Preview	https://deploy-preview-40--confidential-tokens.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

.github/actions/setup/action.yml

Amxx · 2025-06-27T13:10:50Z

contracts/governance/utils/VotesConfidential.sol

+     * @dev Returns the current total supply of votes as an encrypted uint64 (euint64). Must be implemented
+     * by the derived contract.
+     */
+    function totalSupply() public view virtual returns (euint64);


I'm confused by the distinction between this and getCurrentTotalSupply.

should it be internal ?

I would agree on making it internal to avoid specifying override in derived contracts

Amxx · 2025-06-27T13:15:05Z

contracts/governance/utils/VotesConfidential.sol

+    function delegateBySig(
+        address delegator,
+        address delegatee,
+        uint256 nonce,
+        uint256 expiry,
+        bytes memory signature
+    ) public virtual {


I understand the rational for this function having delegator as an argument, and using bytes signatures (instead of v,r,s) ... But I'm wondering if we shouldn't keep the traditional votes function here.

Lets discuss that in a call.

Amxx · 2025-06-27T13:16:57Z

contracts/governance/utils/VotesConfidential.sol

+     */
+    function _transferVotingUnits(address from, address to, euint64 amount) internal virtual {
+        if (from == address(0) || to == address(0)) {
+            _push(_totalCheckpoints, totalSupply());


This is a VERY different behavior from the non-confidential Votes.sol.

Is that on purpose? Is there a security issue in replicating Votes's behavior ?

It's quite expensive to perform the total supply math twice (external calls), as is done in normal votes. I prefer to inherit the total supply from the token contract.

Is the logic that instead of adding and subtracting the supply, we just push the latest totalSupply because it's assumed to be correct?

I think that makes sense, but now the order of calling _transferVotingUnits matter. While I would agree on keeping it like this, I'm worried that calling this function before an _update wouldn't be caught easily

Amxx · 2025-06-27T13:21:52Z

contracts/governance/utils/VotesConfidential.sol

+            }
+            if (to != address(0)) {
+                store = _delegateCheckpoints[to];
+                euint64 newValue = store.latest().add(amount);


store.latest() might be 0 as in "was never created/allocated". These should have special treatment

I believe https://github.com/OpenZeppelin/openzeppelin-confidential-contracts/blob/master/contracts/utils/TFHESafeMath.sol#L16-L25 would be usefull here.

That makes me think the Votes.sol construction, with a _push function that takes a fnPointer, would probably work fine with the functions in THFESafeMath.sol

Didn't use this method due to the reason here #40 (comment)

The linked comment is about totalSupply(). AFAIK, this has nothing to do with the _push method that uses a fnPointer. Are you maybe pointing to the wrong comment ?

And my original comment still stand !

If the trace is empty (that is the case for all addresses that never received any delegation), then store.latest() does

function latest(TraceEuint64 storage self) internal view returns (euint64) { return euint64.wrap(bytes32(self._inner.latest())); }

with self._inner.latest() returning 0.

My question was: is euint64.wrap(0) guaranteed to represent an encrypted 0, and is it ok to use it in FHE.add ???

Ah I posted the wrong response here. The FHE library by Zama always handles euint64.wrap(0) correctly since 0.7.0.

what about other FHE beside zama, should we expect them to do the same ?

https://github.com/zama-ai/fhevm/blob/724b52d65f919d50ca22093adbf141bd38554814/library-solidity/lib/FHE.sol#L3665-L3673

Right now we are tightly coupled to the FHE library produced by Zama. If we were to decouple, we'd have to rethink all operations--any precautions we take here would need to be handled on a per library basis which isn't necessary for Zama.

contracts/governance/README.adoc

ernestognw

I understand the main discussion point is about calculating the totalSupply (or using expensive methods from CheckpointsConfidential). Is that correct? I would encourage generating some quick benchmark to understand the actual cost, that would help us form a better opinion

contracts/governance/utils/VotesConfidential.sol

ernestognw · 2025-07-03T14:22:41Z

contracts/governance/utils/VotesConfidential.sol

+     * @dev Returns the current total supply of votes as an encrypted uint64 (euint64). Must be implemented
+     * by the derived contract.
+     */
+    function totalSupply() public view virtual returns (euint64);


I would agree on making it internal to avoid specifying override in derived contracts

ernestognw · 2025-07-03T14:23:47Z

contracts/governance/utils/VotesConfidential.sol

+    }
+
+    /// @dev Delegates votes from to `delegatee` by sig.
+    function delegateBySig(


Maybe we should note that it only supports ECDSA purposely, as any other smart account should rely on another mechanism for sponsoring

contracts/governance/utils/VotesConfidential.sol

ernestognw · 2025-07-03T14:30:04Z

contracts/governance/utils/VotesConfidential.sol

+     */
+    function _transferVotingUnits(address from, address to, euint64 amount) internal virtual {
+        if (from == address(0) || to == address(0)) {
+            _push(_totalCheckpoints, totalSupply());


Is the logic that instead of adding and subtracting the supply, we just push the latest totalSupply because it's assumed to be correct?

I think that makes sense, but now the order of calling _transferVotingUnits matter. While I would agree on keeping it like this, I'm worried that calling this function before an _update wouldn't be caught easily

arr00 · 2025-07-03T14:41:53Z

@ernestognw can't reply above but in reference to #40 (comment). Yes exactly. Duplicating the math (as is done in vanilla) is very expensive so I want to reuse it. And yes the order matters now for when overriding _update.

Co-authored-by: Ernesto García <ernestognw@gmail.com>

ernestognw

LGTM. I agree with the tradeoffs about reusing storage rather than recomputing

james-toussaint · 2025-07-04T12:53:11Z

contracts/governance/utils/VotesConfidential.sol

+     */
+    function _moveDelegateVotes(address from, address to, euint64 amount) internal virtual {
+        CheckpointsConfidential.TraceEuint64 storage store;
+        if (from != to && euint64.unwrap(amount) != 0) {


if (from != to && amount.isInitialized()) { (& everywhere).
I can create a PR @arr00 if you agree with that (?)

Feel free to create a PR. Seems like isInitialized does the same thing but is a nicer semantic.

arr00 force-pushed the feat/votes branch from db02f88 to 18e1ae1 Compare May 28, 2025 22:15

arr00 closed this Jun 5, 2025

arr00 reopened this Jun 5, 2025

arr00 closed this Jun 9, 2025

arr00 reopened this Jun 9, 2025

arr00 commented Jun 11, 2025

View reviewed changes

.github/actions/setup/action.yml Outdated Show resolved Hide resolved

arr00 requested a review from a team as a code owner June 11, 2025 14:32

arr00 removed the request for review from a team June 11, 2025 14:32

arr00 force-pushed the feat/votes branch from fbcc625 to 71efe8c Compare June 18, 2025 20:42

arr00 changed the base branch from master to feat/checkpoints June 18, 2025 20:43

arr00 force-pushed the feat/checkpoints branch from bfe2b05 to 616bc78 Compare June 25, 2025 18:16

Base automatically changed from feat/checkpoints to master June 26, 2025 12:46

arr00 force-pushed the feat/votes branch from dcb372e to e70cae1 Compare June 26, 2025 21:19

arr00 changed the title ~~WIP: Confidential Votes~~ Votes Confidential Jun 26, 2025

Amxx reviewed Jun 27, 2025

View reviewed changes

arr00 force-pushed the feat/votes branch from b4430da to 4848785 Compare June 27, 2025 17:28

arr00 added 10 commits June 27, 2025 14:17

Confidential Votes WIP

744eb94

up

34ce9ec

add testing for confidential fungible token

3135c02

remove .only

acd8d96

fix coverage

768a570

increase coverage

855a808

reorder functions and add docs

b32b147

add by sig delegation

735b161

add testing to delegate by sig

b5cfe3c

reuse existing total supply handles

32d8a27

just use total supply

cd18298

arr00 requested a review from ernestognw July 2, 2025 19:29

arr00 commented Jul 2, 2025

View reviewed changes

contracts/governance/README.adoc Outdated Show resolved Hide resolved

Update contracts/governance/README.adoc

6c71636

arr00 commented Jul 2, 2025

View reviewed changes

contracts/governance/README.adoc Outdated Show resolved Hide resolved

arr00 added 3 commits July 2, 2025 16:58

Update contracts/governance/README.adoc

b3fc9f1

don't pass nonce unnecessarily

1459148

just ecdsa for delegate by sig

2d089eb

ernestognw reviewed Jul 3, 2025

View reviewed changes

arr00 and others added 3 commits July 3, 2025 10:47

Update contracts/governance/utils/VotesConfidential.sol

6b35c00

Co-authored-by: Ernesto García <ernestognw@gmail.com>

Update contracts/governance/utils/VotesConfidential.sol

c870fe4

Co-authored-by: Ernesto García <ernestognw@gmail.com>

Add warning of when to call _transferVotingUnits

7483690

ernestognw mentioned this pull request Jul 3, 2025

Consider passing delegator and bytes memory signature to VotesConfidential's delegateBySig #76

Open

ernestognw previously approved these changes Jul 3, 2025

View reviewed changes

feedback

1aa2eec

arr00 dismissed ernestognw’s stale review via 1aa2eec July 3, 2025 19:11

arr00 merged commit b7d8fc2 into master Jul 3, 2025
12 checks passed

arr00 deleted the feat/votes branch July 3, 2025 19:37

james-toussaint self-requested a review July 4, 2025 12:33

james-toussaint reviewed Jul 4, 2025

View reviewed changes

This was referenced Jul 4, 2025

Release v5.3.1 (rc) #84

Closed

Release v0.2.0 (rc) #86

Merged

Release v0.2.0 (rc) #88

Merged

Release v0.2.0 (rc) #99

Merged

This was referenced Jul 12, 2025

Release v0.2.0 (rc) #108

Merged

Release v0.2.0 (rc) #110

Merged

github-actions bot mentioned this pull request Aug 5, 2025

Release v0.2.0 #145

Merged

github-actions bot mentioned this pull request Oct 9, 2025

Release v0.3.0 (rc) #221

Merged

github-actions bot mentioned this pull request Nov 12, 2025

Release v0.3.0 #253

Merged

Votes Confidential #40

Votes Confidential #40

Uh oh!

Conversation

arr00 commented May 19, 2025

Uh oh!

netlify bot commented Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for confidential-tokens ready!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ernestognw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arr00 commented Jul 3, 2025

Uh oh!

ernestognw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

netlify bot commented Jun 5, 2025 •

edited

Loading