[Wallet] Add display names of scanned QRs to cache #5154

gnardini · 2020-09-23T05:50:54Z

Description

When scanning a QR, the link embedded on the QR has a display name. This PR stores that info on a cache to use it on the activity feed to show the name of the person we scanned the QR from.

Tested

Read a QR code to make a transfer and saw the display name of the scanned QR on the transactions feed.

Related issues

Fixes Users SBAT see the name of a person in a QR code transaction in the feed #1005
Part of Quality / bug fixing #4835

tarikbellamine · 2020-09-24T21:34:26Z

packages/mobile/src/send/utils.ts

@@ -171,21 +180,44 @@ export function showLimitReachedError(
  return showError(ErrorMessages.PAYMENT_LIMIT_REACHED, ALERT_BANNER_DURATION, translationParams)
 }

+// exported for testing
+export function* _updateRecipientCache(recipient: RecipientWithQrCode) {


Please correct me if I am wrong but I don't think we use the underscore convention as part of the wallet code base. I understand you want to show that this is a private helper function but I don't want it to be confusing because we aren't doing it anywhere else

It's not used a lot but we mention this in our TypeScript style guide

So should we try to actively enforce this moving forward? There are plenty of places where we are exporting for testing only but I've never seen the underscore pattern used.

I'm happy either way but think we should strive for consistency.

tarikbellamine · 2020-09-24T21:34:29Z

packages/mobile/src/send/utils.ts

+  recipientCache[recipient.e164PhoneNumber] = {
+    contactId: '',
+    ...recipient,
+    kind: RecipientKind.Contact,
+  }
+  addressToE164Number[recipient.address] = recipient.e164PhoneNumber


This makes me nervous because it overwrites any existing mapping without having any type of check for security or correctness. I don't know the best way to mitigate this concern atm.

I am imagining a scenario where someone creates an account with a popular but unverified phone number. Anyone that scan's that QR, will have that phone number overwritten with the QR owner's address even though the phone number is not verified

Yeah, I agree, I was a bit lazy tbh, I found this code in some old PR and took it from there. I don't think we should be using phone numbers from this without checking if they're verified, how about this:

Instead of reusing these caches that use phone numbers, create a new one that maps address -> display name that's used as fallback if none of the others have info about the transfer. This way the display name in the QR would only be used if the alternative is to show 'Unknown' in the feed.

Did the changes I suggested above, let me know what you think :)

I like it! I think this is a good short-term solution until we get CIP8 integrated. Please remind whoever implements the CIP8 work to remove this cache.

tarikbellamine

Left a couple of comments.

How would you feel about holding off on this PR until the next release? Personally, I don't feel like we've worked out the UX considerations enough to make this a "must have" for this release.

gnardini · 2020-09-24T22:10:44Z

How would you feel about holding off on this PR until the next release?

No strong opinion, sounds good to me :)

i1skn

Great work! LGTM

jeanregisser

LGTM 👍

jeanregisser · 2020-10-13T13:54:35Z

packages/mobile/src/identity/reducer.ts

@@ -42,6 +42,10 @@ export interface AddressToDataEncryptionKeyType {
  [address: string]: string | null // null means no DEK registered
 }

+export interface AddressToDisplayNameType {
+  [address: string]: string


Suggested change

[address: string]: string

[address: string]: string | undefined

jeanregisser · 2020-10-13T14:00:36Z

packages/mobile/src/send/utils.ts

+  yield put(addAddressToDisplayName(recipient.address, recipient.displayName))
  yield put(storeLatestInRecents(recipient))


nit: I think it's an anti-pattern to dispatch multiple actions consecutively, but we're guilty of this in other places 😢
This is because we have a bunch of actions that are just setters.

See https://redux.js.org/style-guide/style-guide#model-actions-as-events-not-setters

We can reuse the storeLatestInRecents action to avoid sending two with the same parameter :)

jeanregisser · 2020-10-13T14:40:43Z

packages/mobile/src/identity/reducer.ts

+  // Doesn't contain all known addresses, use only as a fallback.
+  addressToDisplayName: AddressToDisplayNameType


Could you add a TODO note about removing this once we have CIP8 working in the app?

jeanregisser · 2020-10-13T14:42:54Z

packages/mobile/__mocks__/react-i18next.js

+// This is useful to test that correct params were sent to i18n.
+// For example, in the TransferFeedItem tests we are checking that the title of the item matches a cached value.
+// Without this it's impossible to check if the used value is the cached one since it only prints the i18n key.
+const printParamInsteadOfKey = {
+  feedItemSentTitle: 'nameOrNumber',
+}
+const translationFunction = (key, params) => {
+  const paramToPrint = printParamInsteadOfKey[key]
+  return paramToPrint ? params[paramToPrint] || key : key
+}
+
+const useMock = [translationFunction, {}]
+useMock.t = translationFunction


Nothing to do now, but it looks like we should revisit the decision of printing only the keys during unit test.
And print the actual EN strings.
What do you think?

That does sound like it would be better, it's one of the things I considered but I think there were some issues around values that may change (dates maybe? I think there was something else). They can be definitely normalised though and it'd be nice to use real strings :)

Lss-Ankit · 2020-10-26T07:51:26Z

Hi @gnardini We have verified task on latest Android play store internal build v1.3.0(1004294316) & Test Flight build v1.3.0(27) and found the following.

Scanned user name is shown on transaction feed when user sending an amount by scanning QR code.
Also, verify that User name is shown on transaction feed for receiver as well.

However, we have updated below issues related to name as Unknown text is shown on transaction feed and profile pic as blank profile pic is shown:

Please let us know if we need to verify any thing else.
Thanks!

[Wallet] Add display names of scanned QRs to cache

fe60479

gnardini added the wallet label Sep 23, 2020

gnardini requested review from cmcewen, i1skn, jeanregisser and jmrossy as code owners September 23, 2020 05:50

Merge branch 'master' into feed-names

8009d19

tarikbellamine reviewed Sep 24, 2020

View reviewed changes

Gonzalo Nardini added 2 commits September 25, 2020 01:31

Use addressToDisplayName as cache

a6d83ce

Add tests for cached values on TransferFeedItem

4451207

tarikbellamine added the do not merge label Sep 25, 2020

gnardini removed the do not merge label Oct 13, 2020

Gonzalo Nardini added 2 commits October 13, 2020 10:04

Merge branch 'master' into feed-names

c645f3e

put instead of call

c8ffff2

i1skn approved these changes Oct 13, 2020

View reviewed changes

jeanregisser approved these changes Oct 13, 2020

View reviewed changes

Reuse action, don't create setter action

55d8c0a

gnardini added the automerge Have PR merge automatically when checks pass label Oct 13, 2020

Merge branch 'master' into feed-names

d68ce4c

mergify bot merged commit c2974ff into master Oct 13, 2020

mergify bot deleted the feed-names branch October 13, 2020 22:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Wallet] Add display names of scanned QRs to cache #5154

[Wallet] Add display names of scanned QRs to cache #5154

gnardini commented Sep 23, 2020

tarikbellamine Sep 24, 2020

jeanregisser Oct 13, 2020

tarikbellamine Oct 13, 2020

tarikbellamine Sep 24, 2020

gnardini Sep 24, 2020

gnardini Sep 25, 2020

tarikbellamine Oct 13, 2020

tarikbellamine left a comment

gnardini commented Sep 24, 2020

i1skn left a comment

jeanregisser left a comment

jeanregisser Oct 13, 2020

jeanregisser Oct 13, 2020

gnardini Oct 13, 2020

jeanregisser Oct 13, 2020

jeanregisser Oct 13, 2020

gnardini Oct 13, 2020

Lss-Ankit commented Oct 26, 2020 •

edited

Loading

	[address: string]: string
	[address: string]: string \| undefined

		yield put(addAddressToDisplayName(recipient.address, recipient.displayName))
		yield put(storeLatestInRecents(recipient))

		// Doesn't contain all known addresses, use only as a fallback.
		addressToDisplayName: AddressToDisplayNameType

[Wallet] Add display names of scanned QRs to cache #5154

[Wallet] Add display names of scanned QRs to cache #5154

Conversation

gnardini commented Sep 23, 2020

Description

Tested

Related issues

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarikbellamine left a comment

Choose a reason for hiding this comment

gnardini commented Sep 24, 2020

i1skn left a comment

Choose a reason for hiding this comment

jeanregisser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lss-Ankit commented Oct 26, 2020 • edited Loading

Lss-Ankit commented Oct 26, 2020 •

edited

Loading