-
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
Conversation
|
You can find the image built from this PR at Built from f1c9b9b |
|
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
| 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" | |
| ) |
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
valueOrorisOkOr:)
The membershipRegisteredLog is of type Option which doesn't seem to work with valueOr or isOkOr, they work with Result
It doesn't quite make sense to me to make membershipRegisteredLog of type Result.
I'll try to see if this section of code can be reworked to fit the required coding style better.
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
|
This PR addresses this issue #3654 |
@Ivansete-status what would be the preferred workflow for unzipping?
|
|
This is the best option IMO:
|
Implemented in 206b7d5 |
|
@Ivansete-status and @fcecin please review latest changes. |
|
@fcecin do you perhaps know why the ci tests are again not running? |
EDIT 2: Actually it's an error: https://github.com/logos-messaging/nwaku/actions/runs/19966186883 EDIT 3: I think the "waku" name is being nuked on other places so |
|
Relates to milestone #3483 |
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.
Superb! Thanks so much for it! 🥳
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.
Related Issue
closes #3654