Skip to content
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

PoC: onchain rln tree + root #31

Closed
wants to merge 25 commits into from
Closed

PoC: onchain rln tree + root #31

wants to merge 25 commits into from

Conversation

rymnc
Copy link
Contributor

@rymnc rymnc commented Nov 29, 2023

Description

This PR includes 2 important changes -

  • Integrate BinaryIMT (edit LazyIMT) into the Rln contract to allow for trustless root computation for lighter verifiers, reference: Onchain RLN + L2 waku-org/research#56
  • Moves the codebase over the the vacp2p/foundry-template, since verification of these contracts is failing due to an etherscan bug between etherscan <> hardhat-deploy. Saw this as an opportunity to port the repo.

Apologies for the large PR, but please note that most of the changes are restricted to Rln.sol, and a new function computeRoot. The tests remain the same, with some additional testing for compatibility with zerokit.

Copy link

@alrevuelta alrevuelta left a comment

Choose a reason for hiding this comment

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

Thanks! Left some comments.

contracts/PoseidonHasher.sol Outdated Show resolved Hide resolved
test/Rln.t.sol Outdated Show resolved Hide resolved
contracts/RlnBase.sol Outdated Show resolved Hide resolved
contracts/RlnBase.sol Outdated Show resolved Hide resolved
contracts/RlnBase.sol Show resolved Hide resolved
Copy link

LCOV of commit ea4a157 during CI #49

Summary coverage rate:
  lines......: 30.4% (41 of 135 lines)
  functions..: 52.0% (13 of 25 functions)
  branches...: 52.5% (21 of 40 branches)

Files changed coverage rate:
                              |Lines       |Functions  |Branches    
  Filename                    |Rate     Num|Rate    Num|Rate     Num
  ==================================================================
  contracts/PoseidonHasher.sol| 100%      1| 100%     1|    -      0
  contracts/RlnBase.sol       | 100%     37| 100%     8| 100%     18

@rymnc
Copy link
Contributor Author

rymnc commented Nov 30, 2023

@alrevuelta as requested,

{
  "RLN": {
    "polygonZkevmTestnet": "https://testnet-zkevm.polygonscan.com/address/0xC568eF58009b8e5B8824d9fbB271141782545538#",
    "sepolia": "https://sepolia.etherscan.io/address/0xbE24C8d709754523D882D4b67C59e983107cf1E8"
  }
}

Copy link

github-actions bot commented Dec 1, 2023

LCOV of commit caa3d3b during CI #51

Summary coverage rate:
  lines......: 29.9% (40 of 134 lines)
  functions..: 50.0% (12 of 24 functions)
  branches...: 52.5% (21 of 40 branches)

Files changed coverage rate:
                           |Lines       |Functions  |Branches    
  Filename                 |Rate     Num|Rate    Num|Rate     Num
  ===============================================================
  contracts/RlnBase.sol    | 100%     37| 100%     8| 100%     18
  test/RLNApp.t.sol        | 100%      2| 100%     2|75.0%      4

Copy link

github-actions bot commented Dec 1, 2023

LCOV of commit a3193ce during CI #52

Summary coverage rate:
  lines......: 33.8% (48 of 142 lines)
  functions..: 52.0% (13 of 25 functions)
  branches...: 52.3% (23 of 44 branches)

Files changed coverage rate:
                           |Lines       |Functions  |Branches    
  Filename                 |Rate     Num|Rate    Num|Rate     Num
  ===============================================================
  contracts/RlnBase.sol    | 100%     45| 100%     9|90.9%     22
  test/RLNApp.t.sol        | 100%      2| 100%     2|75.0%      4

test/Rln.t.sol Outdated

function test__root() public {
uint256[] memory idCommitments = new uint256[](10);
idCommitments[0] = 19143711682366759980911001457853255795836264632723844153354310748778748156460;

Choose a reason for hiding this comment

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

no action point here. leaving this for reference, in case we want to generate other testvectors. isn't golang beautiful?

package main

import (
	"math/big"

	log "github.com/sirupsen/logrus"
	"github.com/waku-org/go-zerokit-rln/rln"
)

func main() {

	rlnInstance, err := rln.NewRLN()
	if err != nil {
		log.Fatal(err)
	}

	leafsTestVector := []*big.Int{
		ParseBigInt("19143711682366759980911001457853255795836264632723844153354310748778748156460"),
		ParseBigInt("16984765328852711772291441487727981184905800779020079168989152080434188364678"),
		ParseBigInt("10972315136095845343447418815139813428649316683283020632475608655814722712541"),
		ParseBigInt("2709631781045191277266130708832884002577134582503944059038971337978087532997"),
		ParseBigInt("8255654132980945447086418574686169461187805238257784695584517016324877809505"),
		ParseBigInt("20291701150251695209910387548168084091751201746043024067531503187703236470983"),
		ParseBigInt("11817872986033932471261438074921403500882957864164537515599299873089437746577"),
		ParseBigInt("18475838919635792169148272767721284591038756730004222133003018558598315558783"),
		ParseBigInt("10612118277928165031660389522171737855229037400929675201853245490188277695983"),
		ParseBigInt("17318633845296358766427229711888486415250435256643711009388405482885762601797"),
	}

	for _, leaf := range leafsTestVector {
		rlnInstance.InsertMember(rln.BigIntToBytes32(leaf))
	}

	root, err := rlnInstance.GetMerkleRoot()
	if err != nil {
		log.Fatal(err)
	}

	log.Info("Merkle root:", rln.Bytes32ToBigInt(root))
	// 5210724218081541877101688952118136930297124697603087561558225712176057209122
}

func ParseBigInt(s string) *big.Int {
	x, ok := new(big.Int).SetString(s, 10)
	if !ok {
		log.Fatal("could not parse")
	}
	return x
}

@rymnc rymnc marked this pull request as ready for review December 5, 2023 09:59
@rymnc rymnc requested a review from alrevuelta December 5, 2023 09:59
@rymnc rymnc self-assigned this Dec 5, 2023
Copy link
Contributor

@0x-r4bbit 0x-r4bbit left a comment

Choose a reason for hiding this comment

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

Massive work!
This is still in draft, but I left some comments anyways

.gas-snapshot Outdated
@@ -0,0 +1 @@
FooTest:test_Example() (gas: 8662)
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably want to have this regenerated based on the tests of this repository.
Just need to run forge snapshot

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed in f22afdf


# Rename instances of "vacp2p/foundry-template" to the new repo name in README.md for badges only
sedi "/gha/ s|vacp2p/foundry-template|"${GITHUB_REPOSITORY}"|;" "README.md"
sedi "/gha-badge/ s|vacp2p/foundry-template|"${GITHUB_REPOSITORY}"|;" "README.md"
Copy link
Contributor

Choose a reason for hiding this comment

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

This entire file is not needed and is only triggered when forking/creating a repo from our template repo.
So you can remove this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 9f3b5df

- name: "Add commit summary"
run: |
echo "## Commit result" >> $GITHUB_STEP_SUMMARY
echo "✅ Passed" >> $GITHUB_STEP_SUMMARY
Copy link
Contributor

Choose a reason for hiding this comment

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

This file is also only needed for forking/creating from the GH template.
No need to keep this here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 9f3b5df

LICENSE.md Outdated
THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE
WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR
COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this needs to be changed. Probably fine to just keep it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 9f3b5df, we have the licenses as LICENSE-MIT and LICENSE-APACHE

[foundry]: https://getfoundry.sh/
[foundry-badge]: https://img.shields.io/badge/Built%20with-Foundry-FFDB1C.svg
[license]: https://opensource.org/licenses/MIT
[license-badge]: https://img.shields.io/badge/License-MIT-blue.svg
Copy link
Contributor

Choose a reason for hiding this comment

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

We also have a badge for coverage. If you want to have this added, let me know

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not at the moment!

foundry.toml Outdated
bytecode_hash = "none"
cbor_metadata = false
evm_version = "shanghai"
Copy link
Contributor

Choose a reason for hiding this comment

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

Unless you're sure that these contracts aren't going to be deployed on chains that don't support for example PUSH0, I'd use "paris" for now.

For more info, see https://www.evmdiff.com/diff?base=1&target=10

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 700d99b

"version": "1.0.0",
"author": {
"name": "0x-r4bbit",
"url": "https://github.com/vacp2p"
Copy link
Contributor

Choose a reason for hiding this comment

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

This part probably needs change (name, description, author)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in a647a84


function getOrCreatePolygonZkevmConfig() public view returns (NetworkConfig memory) {
return NetworkConfig({ deployer: deployer });
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Unless there's a plan to actually provide different configurations pre network, we can probably remove these and simplify the constructor. But I'm also fine if we keep it for explicitness

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Leaving it in for explicitness if we want to change it!

fi


forge script script/Deploy.s.sol:Deploy --chain $chain_name --rpc-url $rpc_url --private-key "$PRIVATE_KEY" --broadcast -v
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI: Since we're using Deploy.s.sol here, which is BaseScript, we can probably better use the MNEMONIC environment variable over --private-key.

Otherwise, I believe there's also an interactive mode in forge script using -i which lets you enter the private key. Not sure if this is any better though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

err, i dont use a mnemonic to deploy, and if anyone needs to, they can always go back to deriving a private key for it, not the other way around. seems fine to me for now

@alrevuelta alrevuelta changed the title computeRoot view function PoC: onchain rln tree + root Dec 5, 2023
Copy link

@alrevuelta alrevuelta left a comment

Choose a reason for hiding this comment

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

No more input on my side by now. This was enough for a Proof of Concept (see waku-org/research#72) and until we decide if we want to integrate it or not in The Waku Network, we can consider it done.

If at some point we decide to move forward, we will have to do proper/deep reviews.

By now I would leave the PR open for reference, with no intention to merge by now.

Thanks!

@alrevuelta
Copy link

Can we close this since the LazyIMT was integrated successfully here?
https://github.com/waku-org/waku-rlnv2-contract

@rymnc rymnc closed this May 31, 2024
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.

3 participants