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

Profiles: use name text record instead of me.rainbow.displayName #4046

Merged
merged 8 commits into from
Sep 2, 2022

Conversation

jxom
Copy link
Contributor

@jxom jxom commented Aug 18, 2022

Fixes TEAM2-432

What changed (plus any additional context for devs)

With the name record to soon be a standard in ENS in a future ENSIP, we should probably use that instead of me.rainbow.displayName. Since we can't migrate ENS text record fields for users without it costing gas, we will still need to support the me.rainbow.displayName field if it is set (and a name is not).

This PR migrates to set the name field when a user is creating/updating their profile. If a user is updating their profile, and already has a me.rainbow.displayName set, when the profile is updated, it will migrate this field to use name instead (delete the me.rainbow.displayName record, and then set the name record).
This PR also adds the ability to still view the me.rainbow.displayName field if a user does not yet have a name field set (via the deprecatedTextRecordFields value).

Screen recordings / screenshots

Here is a quick video showing how a migration works when a user updates their profile (note, the migration will occur for any update to records).

https://www.loom.com/share/976958d2887f4c67b1c9fd974168a34d

If a user has both a name and me.rainbow.displayName field set, it will prefer the name value:

Screen Shot 2022-08-19 at 9 12 41 am

Screen Shot 2022-08-19 at 9 12 58 am

What to test

  • If a user only has me.rainbow.displayName set, ensure you can still view the name on:
    • the View Profile screen
    • the Edit Profile screen
    • the ENS NFT expanded state
  • If a user has both a name and me.rainbow.displayName set, ensure that the name value is preferred on:
    • the View Profile screen
    • the Edit Profile screen
    • the ENS NFT expanded state
  • If a user has a me.rainbow.displayName set, and goes to update any record on their profile, after updating ensure that the me.rainbow.displayName record is removed, and the name record is now set

Final checklist

  • Assigned individual reviewers?
  • Added labels? (team1/team2, critical path, release, dev QA)
  • Did you test both iOS and Android?
  • If no dev QA label, did you add the PR to the QA Queue?

@linear
Copy link

linear bot commented Aug 18, 2022

TEAM2-432 Support `name` instead of `me.rainbow.displayName`

We should ideally use name instead of me.rainbow.displayName as this will be the official standard going forward: https://gist.github.com/TateB/9f9a13574c4cd5ceca82023788fa2548

We will still need to provide backwards compatibility for me.rainbow.displayName so it doesn't break profiles.

@jxom jxom requested review from benisgold, estebanmino, a team, pugson and skylarbarrera August 18, 2022 23:31
@jxom jxom added profiles ENS Profiles team2 labels Aug 18, 2022
Copy link
Contributor

@pugson pugson left a comment

Choose a reason for hiding this comment

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

amazing 🥇 👏

Copy link
Contributor

@skylarbarrera skylarbarrera left a comment

Choose a reason for hiding this comment

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

tested all scenarios, code looks crisp 🍎

PoW: https://cloud.skylarbarrera.com/Screen-Shot-2022-08-19-10-37-34.89.png

e2e/registerENSFlow.spec.js Outdated Show resolved Hide resolved
@@ -369,6 +370,12 @@ export const textRecordFields = {
[key in ENS_RECORDS]?: TextRecordField;
};

export const deprecatedTextRecordFields = {
Copy link
Contributor

Choose a reason for hiding this comment

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

clean af

@@ -408,7 +398,7 @@ describe('Register ENS Flow', () => {
throw new Error('Resolved address is wrong');
if (primaryName !== RAINBOW_WALLET_NAME)
throw new Error('Resolved primary name is wrong');
if (displayName) throw new Error('me.rainbow.displayName name is wrong');
if (name) throw new Error('name is wrong');
Copy link
Contributor

Choose a reason for hiding this comment

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

👌

Copy link
Contributor

@estebanmino estebanmino left a comment

Choose a reason for hiding this comment

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

code is looking good! nice solution to get this to work, just want to make a note that making this will cost a bit more to update the profile, talked with @jxom directly about it 🤝

i also found this https://www.loom.com/share/cecf1699694b4929b24a7775e631634a when migrating the name to the new format is taking the old name Esteban as updated when I edit the name and putting Esteban -> Esteba -> Esteban again, you can see the review button
probably edge case but this doesn't happen on other records

@jxom
Copy link
Contributor Author

jxom commented Aug 25, 2022

I can't seem to reproduce this :\

Kapture.2022-08-25.at.14.40.08.mp4

@estebanmino
Copy link
Contributor

@jxom maybe because i'm doing the migration on the video so i didn't have the new name record? seems like moxey.eth is already migrated

Copy link
Contributor

@estebanmino estebanmino left a comment

Choose a reason for hiding this comment

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

working perfectly! 🚀

@jxom jxom removed the request for review from benisgold August 30, 2022 20:36
@BrodyHughes
Copy link
Member

I was having trouble QAing bc of the bug where collections don't show up, but I think were gonna merge then QA spot check in the release branch for 1.7.4.

@estebanmino estebanmino merged commit 79114e7 into develop Sep 2, 2022
@estebanmino estebanmino deleted the @jxom/team2-432 branch September 2, 2022 18:29
estrattonbailey added a commit that referenced this pull request Sep 6, 2022
…w-4378-clean-up-global-and-env-vars-in-app-move

* 'develop' of github.com:rainbow-me/rainbow: (40 commits)
  fix: quiet logs when linting in CI (#4070)
  analytics: add tracking for screen dimensions + fix nav events on android (#4088)
  fix native input cursor color (#4112)
  Profiles: use `name` text record instead of `me.rainbow.displayName` (#4046)
  chore: upgrade eslint-config-rainbow (#4118)
  backup PIN  (#4012)
  Remove long press on address in switching wallets (#3990)
  Version bump iOS v.1.7.4 & Android 144 (#4117)
  Fix slow image loading (#4052)
  Add new text size scale, deprecate old sizes and Heading (#4113)
  Disable campaings in e2e and fix testID of PromoSheet (#4107)
  🏛 chore: prettier everything (#4100)
  fix settings StatusIcon shadows on android (#4104)
  fix clear async storage crash (#4105)
  fix RNBW-4416 (#4103)
  🏛 Update to React Query 4 (#4060)
  swaps: init cross chain feature flag (#4091)
  enable arbiitrum swap details test (#4092)
  remove unsafe access to networkInfo (#4096)
  network check (#4094)
  ...
estrattonbailey added a commit that referenced this pull request Sep 6, 2022
…w-4388-configure-jest-for-unit-tests

* 'develop' of github.com:rainbow-me/rainbow:
  Profiles: Support contenthash (#4067)
  🏛 GraphQL Clients/Codegen Foundations (#4086)
  fix: quiet logs when linting in CI (#4070)
  analytics: add tracking for screen dimensions + fix nav events on android (#4088)
  fix native input cursor color (#4112)
  Profiles: use `name` text record instead of `me.rainbow.displayName` (#4046)
  chore: upgrade eslint-config-rainbow (#4118)
  backup PIN  (#4012)
  Remove long press on address in switching wallets (#3990)
  Version bump iOS v.1.7.4 & Android 144 (#4117)
  Fix slow image loading (#4052)
  Add new text size scale, deprecate old sizes and Heading (#4113)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
profiles ENS Profiles team2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants