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

chore(rln-relay): integrate waku rln registry #1943

Merged
merged 1 commit into from
Aug 25, 2023
Merged

Conversation

rymnc
Copy link
Contributor

@rymnc rymnc commented Aug 24, 2023

Description

This PR integrates waku's fork of the rln contracts - https://github.com/waku-org/waku-rln-contract

Which brings the registry, that serves as an entrypoint into the waku rln membership set, which can have multiple trees located within them

Changes

  • Removes persistCredentials from OnchainGroupManager
  • Removes the onchain registration from startGroupSync
  • Updates the bytecode of PoseidonHasher and RegistryContractCode
  • Handles the usingStorageIndex appropriately and subscribes to the usingStorageIndex tree

This PR lays the foundation for 2 items -

  1. sync from deployed block number (bug + chore: Unhandled exception + fix reducing blockChunkSize #1942)
  2. optional user input for rln storage contract slot

@github-actions
Copy link

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:1943

@rymnc rymnc self-assigned this Aug 24, 2023
@rymnc rymnc marked this pull request as ready for review August 24, 2023 17:34
Copy link
Contributor

@jm-clius jm-clius left a comment

Choose a reason for hiding this comment

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

LGTM as far as I could follow :)

@vpavlin
Copy link
Member

vpavlin commented Aug 25, 2023

Out of curiosity - why is it removing the persistCredentials?

Copy link
Member

@vpavlin vpavlin left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Collaborator

@Ivansete-status Ivansete-status left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -168,18 +165,16 @@ proc setup(signer = true): Future[OnchainGroupManager] {.async.} =
let web3 = await newWeb3(EthClient)

let accounts = await web3.provider.eth_accounts()
web3.defaultAccount = accounts[1]
web3.defaultAccount = accounts[0]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shall we check the number of accounts is > 0?

Suggested change
web3.defaultAccount = accounts[0]
assert accounts.len > 0, "Not enough accounts retrieved"

Copy link
Member

Choose a reason for hiding this comment

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

Not neccesary (AFAIU:) ) - the web3.provider.eth_accounts() is an RPC call to an ETH node which will return it the accounts that it knows of (in case of ganache it should be something liek 10 default well known accounts/wallets).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

correct, the eth_accounts will always be > 0, since we're using a test rpc provider - ganache

# proc registerBatch(pubkeys: seq[Uint256]) # external payable
# proc withdraw(secret: Uint256, pubkeyIndex: Uint256, receiver: Address)
# proc withdrawBatch( secrets: seq[Uint256], pubkeyIndex: seq[Uint256], receiver: seq[Address])
proc MEMBERSHIP_DEPOSIT(): Uint256 {.pure.}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Out of curiosity: why upper case :) ?

Copy link
Member

Choose a reason for hiding this comment

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

This Nim file is just a wrapper for the contract ABI and in Solidity constants and immutable stored variables are usually capitalized - this particular "proc" is just a representation an automatically generated getter based on https://github.com/vacp2p/rln-contract/blob/bd8403a74e327707afb70e60f582e2546e487891/contracts/RlnBase.sol#L45

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right on point @vpavlin !

@rymnc rymnc merged commit cc9f8d4 into master Aug 25, 2023
23 checks passed
@rymnc rymnc deleted the integrate-waku-rln branch August 25, 2023 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants