chore: group ens names with chain-id #18001
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Jenkins BuildsClick to see older builds (16)
|
cammellos
approved these changes
Nov 27, 2023
69% of end-end tests have passedFailed tests (9)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMergedTwo:
Expected to fail tests (5)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (31)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMergedTwo:
|
flexsurfer
approved these changes
Nov 27, 2023
qfrank
force-pushed
the
adapt-ens-names-data-structure
branch
from
7dfbd31 to
662e140
Compare
OmarBasem
approved these changes
Nov 28, 2023
qfrank
force-pushed
the
adapt-ens-names-data-structure
branch
from
662e140 to
63b640c
Compare
qfrank
force-pushed
the
adapt-ens-names-data-structure
branch
2 times, most recently
from
355764f to
4cd1051
Compare
75% of end-end tests have passedFailed tests (9)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Expected to fail tests (3)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Passed tests (36)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Class TestDeepLinksOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePRTwo:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
|
|
Hi @qfrank Thank you for PR. Take a look please a found issues: Should the user's ENS still be visible across different networks after switching networks? Question 1/ ISSUE 1 Displaying user's ENS across different networksSteps to Reproduce:
Actual Result:The user's ENS is still displayed within chats, profile, and after sending messages, even after switching networks. Expected Result:The user's ENS should not be visible or displayed after switching to a different network Additional info:Please let me know if you don't have an account with ENS on goerli and I can share the seed phrase via private message Logs: |
ISSUE 2: ENS resolution purchased on Goerli for others users across mainnet networkSteps:
Actual result:The ENS associated with Expected result:The ENS of Logs:User A logs: User B logs: |
78% of end-end tests have passedFailed tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Expected to fail tests (1)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (7)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
|
|
that's ok if this issue is not in scope of this PR and will be fixed in separately ISSUE 3: Identifier rings not displayed for other users in different networksSteps to Reproduce:
Actual Result:Identifier rings representing Expected Result:If the user's display name is shown, the identifier rings should also be displayed Logs:
|
|
Hi @VolodLytvynenko , thank you for your feedback, issue 1-3 is not an issue IMO, they're relate to how ENS get verified. cc @cammellos
|
@qfrank Understood. I thought this PR also addresses the issue of displaying Goerli ENS for mainnet users. As described in issue 2 the ENS purchased on Goerli is visible for users who are on the mainnet. |
sorry for the confusion , just updated |
@qfrank I think that's ok. Thank you for your. No issues from my side. PR is ready to be merged |
|
Hi @cammellos This PR has been tested and is ready for merging. Could you please review issue 2 and approve it if it's suitable to be not fixed for now? |
|
Hi @qfrank sorry for the delay. Discussed this PR with @cammellos . PR can be merged. Thank you! |
qfrank
force-pushed
the
adapt-ens-names-data-structure
branch
from
4cd1051 to
44e8f32
Compare
yevh-berdnyk
pushed a commit
that referenced
this pull request
Dec 8, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adjust the db store structure to accommodate ENS names by adding a 'chain-id' to group them.
Although we already have above rules implemented, user can still register ens name on different network via switch network.
Additional description:
status-go already implemented chain-id within table
ens_usernames, but status-mobile didn't take care of field chain-id:before my PR, the ens names store in re-frame db may like this:
userA.eth -> {username:userA.eth, chain-id: 1, clock: ...}
userB.eth -> {username:userB.eth, chain-id: 2, clock: ...}
but this could have potential issue like:
what if we got the same ens names, but on different chain-id?
after my PR, the ens names store in re-frame db like this:
1 -> userA.eth -> {username:userA.eth, chain-id: 1, clock: ...}
2 -> userB.eth -> {username:userB.eth, chain-id: 2, clock: ...}
ens names grouped by chain-id.
Testing notes
Platforms
status: ready