-
Notifications
You must be signed in to change notification settings - Fork 78
Chore: bump waku-rlnv2-contract-repo commit #3651
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
base: master
Are you sure you want to change the base?
Conversation
|
You can find the image built from this PR at Built from 6a6b065 |
|
related issue: #3654 |
8eef4d5 to
33ebeb9
Compare
|
TODO:
|
Ivansete-status
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks so much indeed! 💯 🥳
I'm adding some nitpicks that I hope you find useful.
Also:
-
Kindly link this PR with maintenance issue (see how other are linked.)
-
If possible, it would be interesting to push the
tests/waku_rln_relay/anvil_state/state-deployed-contracts-mint-and-approved.jsonfile zipped/compressed so that we keep the repo small when cloning -
I think is also interesting to reduce the
timeout-minutes: 90to 45 in.github/workflows/ci.yml
| const DEFAULT_ANVIL_STATE_PATH* = | ||
| "tests/waku_rln_relay/anvil_state/state-deployed-contracts-mint-and-approved.json" | ||
| const TOKEN_ADDRESS* = "0x5FbDB2315678afecb367f032d93F642f64180aa3" | ||
| const WAKU_RLNV2_PROXY_ADDRESS* = "0x5fc8d32690cc91d4c39d9d3abcbd16989f875707" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kindly add a comment describing each item
| return | ||
|
|
||
| # mint the token from the generated account | ||
| discard await sendMintCall( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe aside from PR but, if always discard, maybe we can just make sendMintCall return nothing?
| let tokenApprovalResult = await approveTokenAllowanceAndVerify( | ||
| web3, acc, privateKey, testTokenAddress, contractAddress, ethToWei(200.u256) | ||
| ) | ||
| assert tokenApprovalResult.isOk, tokenApprovalResult.error() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super nitpick sorry, verbs with parenthesis, and noun without :)
| assert tokenApprovalResult.isOk, tokenApprovalResult.error() | |
| assert tokenApprovalResult.isOk(), tokenApprovalResult.error |
| int(await ethRpc.provider.eth_gasPrice()) * 2 | ||
| let idCommitmentHex = identityCredential.idCommitment.inHex() | ||
| info "identityCredential idCommitmentHex", idCommitment = idCommitmentHex | ||
| debug "identityCredential idCommitmentHex", idCommitment = idCommitmentHex |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Notice that by default we are using INFO ( not saying this is wrong .)
| if membershipRegisteredLog.isNone(): | ||
| raise newException( | ||
| ValueError, "register: MembershipRegistered event not found in transaction logs" | ||
| ) | ||
|
|
||
| let registrationLog = membershipRegisteredLog.get() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If possible, let's better use valueOr or isOkOr :)
| if membershipRegisteredLog.isNone(): | |
| raise newException( | |
| ValueError, "register: MembershipRegistered event not found in transaction logs" | |
| ) | |
| let registrationLog = membershipRegisteredLog.get() | |
| let registrationLog = membershipRegisteredLog.valueOr: | |
| raise newException( | |
| ValueError, "register: MembershipRegistered event not found in transaction logs" | |
| ) |
fcecin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Description
Bumping waku-rlnv2-contract repo to latest commit. This is to fix the CI tests getting stuck on contract deployment.
Changes
Use waku-rlnv2-contract commit
8a338f354481e8a3f3d64a72e38fad4c62e32dcdThe updated contract emits additional events at registration. The method of retrieving the Membership registered event was updated in the group_manager.