-
Notifications
You must be signed in to change notification settings - Fork 265
test: Token gated communities #7113
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: develop
Are you sure you want to change the base?
Conversation
Jenkins BuildsClick to see older builds (387)
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #7113 +/- ##
===========================================
- Coverage 59.98% 59.30% -0.69%
===========================================
Files 813 813
Lines 113409 113409
===========================================
- Hits 68033 67255 -778
- Misses 38517 39339 +822
+ Partials 6859 6815 -44
Flags with carried forward coverage won't be shown. Click here to find out more. |
fbarbu15
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!
igor-sirotin
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.
Thanks @romanzac! 👍
Looks good to me, just a few polishing suggestions
tests-functional/tests/test_wakuext_community_token_permissions.py
Outdated
Show resolved
Hide resolved
tests-functional/tests/test_wakuext_community_token_permissions.py
Outdated
Show resolved
Hide resolved
tests-functional/tests/test_wakuext_community_token_permissions.py
Outdated
Show resolved
Hide resolved
tests-functional/tests/test_wakuext_community_token_permissions.py
Outdated
Show resolved
Hide resolved
tests-functional/tests/test_wakuext_community_token_permissions.py
Outdated
Show resolved
Hide resolved
tests-functional/tests/test_wakuext_community_token_permissions.py
Outdated
Show resolved
Hide resolved
| assert accept_resp is not None, f"Failed to accept request: {accept_resp}" | ||
|
|
||
| # Explicitly reevaluate community members so token-based roles are applied | ||
| self.owner.wakuext_service.reevaluate_community_members_permissions(community_id) |
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.
I think this should be done automatically
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.
Evaluation removed at cbfd86f
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.
This is probably where we can wait for new.messages signal and check CommunityChanges
there's this signal that you should wait for here:
| MemberReevaluationStatus = "community.memberReevaluationStatus" |
tests-functional/tests/test_wakuext_community_token_permissions.py
Outdated
Show resolved
Hide resolved
49ce18a to
d1a9df7
Compare
| # Create token overrides for wallet service | ||
| token_overrides = [{"symbol": "SNT", "address": self.snt_address}] |
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.
token_overrides should probably be applied to all created backends: owner, member ,etc.
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.
I took an approach to deploy and mint tokens on Anvil to get closer to realistic behavior. I've seen Go unit tests mock blockchain responses, so it might be beneficial to increase environment complexity to see how backend handles that.
I understand latency and smart contracts on Anvil aren't fully up to the real world - this is perhaps tested with UI - E2E tests ?
If there is a wallet cache, why do we need overrides ? Does cache miss here come with high latency?
Deleted them for now 3308148 Until we have a conclusion on the test approach.
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.
tbh, I've no idea what TokenOverrides does.
@friofry can you please share your wisdom?
tests-functional/tests/test_wakuext_community_token_permissions.py
Outdated
Show resolved
Hide resolved
tests-functional/tests/test_wakuext_community_token_permissions.py
Outdated
Show resolved
Hide resolved
| def setup_backends(self, backend_new_profile, foundry_client): | ||
| """Initialize backends for token permission tests""" | ||
| self.owner = backend_new_profile("owner") | ||
| self.member = backend_new_profile("member") |
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.
It's not good that we create both member and member_with_snt for all tests, using setup_backends.
self.member is only used in 1/3 tests. And self.member_with_snt is only used in the other 2.
This is a waste of time and also makes it more difficult to debug.
Perhaps we can define fixtures right here, and use them in specific tests? Like owner fixture, etc.
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.
Fixed at 4aa8b34
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.
Hmm, not sure if polluting conftest.py with this is a good idea, I'd rather keep these fixtures right next to the test.
But it's just my opinion, completely up to you.
4aa8b34 to
77e3174
Compare
- test_admin_token_permissions_with_valid_tokens
- cleanup and add additional logging
- test_owner_edits_visible_before_and_after_minting_owner_token
- community.memberReevaluationStatus
Co-authored-by: Igor Sirotin <[email protected]>
77e3174 to
bcd67e6
Compare
- test_membership_no_valid_tokens_fake_address
|
@igor-sirotin I've reintroduced negative test. I think it is still interesting we reject improper requests. When signature does not match ideally even rejecting at API level. Is my assumption correct ? 8c229d4 Or are these negative tests out of scope, because status-go would never act as server for many users? |
Sounds good, let's have it 👍 But instead of verifying Let's keep this test skipped in your PR, I will adapt it in #7178. |
Summary
An initial batch of functional tests to cover token-gated communities.
Tests Included
Issues discovered