Identity Metadata Repository setup #2520

lmuntaner · 2024-06-27T14:11:54Z

Main motivation is to set up the module to manage the anchor metadata.

🟡 Some screens were changed

lmuntaner · 2024-06-27T14:15:04Z

@frederikrothenberger before I continue with tests could you take a look and let me know if this is what you had in mind.

frederikrothenberger

Yes, this is the right direction. Not sure about the complexity of retries though...
It makes the front-end more robust, but the PR #2517 didn't have it either?

frederikrothenberger · 2024-06-27T14:33:21Z

src/frontend/src/repositories/anchorMetadata.ts

+import type { AuthenticatedConnection } from "$src/utils/iiConnection";
+
+export type AnchorMetadata = {
+  recoveryPageShownTimestampMillis?: number;


Doesn't this need to be bigint?

I will get it from the browser with Date.now() which returns a number.

From chatGPT:

Date.now() will return Number.MAX_SAFE_INTEGER sometime around the year 287,586

If we were using nanoseconds, then yes. But with milliseconds it should be enough.

frederikrothenberger · 2024-06-27T14:38:38Z

src/frontend/src/repositories/anchorMetadata.ts

+
+const RECOVERY_PAGE_SHOW_TIMESTAMP_MILLIS = "recoveryPageShownTimestampMillis";
+const METADATA_FETCH_TIMEOUT = 1_000;
+const METADATA_FETCH_RETRIES = 10;


10 seems like a lot, no?

frederikrothenberger · 2024-06-27T14:39:42Z

src/frontend/src/repositories/anchorMetadata.ts

+        break;
+      }
+    }
+    if (


I'd suggest to extract a helper for this condition.

makes sense

frederikrothenberger · 2024-06-27T14:43:43Z

src/frontend/src/utils/iiConnection.ts

@@ -511,6 +521,34 @@ export class AuthenticatedConnection extends Connection {
    await actor.remove(this.userNumber, publicKey);
  };

+  getIdentityInfo = async (): Promise<


A client should only use the getAnchorMetadata, setPartialMetadata and commitMetadata functions. getIdentityInfo and setIdentityMetadata should be private then, no?

Then the repository won't be able to use it...

I could pass the getter and setter as functions only.

lmuntaner · 2024-06-27T15:41:26Z

Yes, this is the right direction. Not sure about the complexity of retries though... It makes the front-end more robust, but the PR #2517 didn't have it either?

I was thinking and yes, I think we don't need retries. Even more, I think I want to swallow the errors and log a warning only.

The metadata is not crucial, we can find sensible defaults in case something isn't working as expected. Moreover, with the imperative code, I don't want an error to get metadata to disrupt a user flow.

@frederikrothenberger What do you think if I remove all the throws and instead swallow or return undefined or false accordingly?

frederikrothenberger · 2024-06-27T15:43:17Z

Yes, this is the right direction. Not sure about the complexity of retries though... It makes the front-end more robust, but the PR #2517 didn't have it either?

I was thinking and yes, I think we don't need retries. Even more, I think I want to swallow the errors and log a warning only.

The metadata is not crucial, we can find sensible defaults in case something isn't working as expected. Moreover, with the imperative code, I don't want an error to get metadata to disrupt a user flow.

@frederikrothenberger What do you think if I remove all the throws and instead swallow or return undefined or false accordingly?

Yes, a warning should be sufficient. 👍

lmuntaner · 2024-06-28T10:52:02Z

src/frontend/src/flows/authorize/index.ts

@@ -212,6 +212,7 @@ const authenticate = async (
  const derivationOrigin =
    authContext.authRequest.derivationOrigin ?? authContext.requestOrigin;

+  // TODO: Commit state


To be done in another PR, but I want to remember exactly where to put it.

lmuntaner · 2024-06-28T10:52:08Z

src/frontend/src/flows/manage/index.ts

@@ -250,6 +250,7 @@ export const renderManage = async ({
  for (;;) {
    let anchorInfo: IdentityAnchorInfo;
    try {
+      // TODO: commit state


To be done in another PR, but I want to remember exactly where to put it.

lmuntaner · 2024-06-28T10:53:17Z

@frederikrothenberger ready for review

frederikrothenberger

Thanks! I still have a few questions.

src/frontend/src/repositories/anchorMetadata.ts

frederikrothenberger · 2024-06-28T11:24:11Z

src/frontend/src/repositories/anchorMetadata.ts

+ * It can then be read and updated in memory.
+ * The metadata needs to be committed to the canister with the `commitMetadata` method to persist the changes.
+ */
+export class AnchorMetadataRepository {


Quite a while ago we decided to sunset the term "anchor" because it is confusing. So on the UI, this term is no longer used. In the code, we didn't change things.
But for new code, we now use "identity" rather than anchor. Hence, why on the API level it is called IdentityMetadata rather than AnchorMetadata.
I think it would be better to use the new terminology here too and keep going with the slow journey of eventually replacing all the occurrences of "anchor".

src/frontend/src/utils/iiConnection.test.ts

frederikrothenberger · 2024-06-28T11:33:32Z

src/frontend/src/utils/iiConnection.test.ts

+  identity_metadata_replace: vi.fn().mockResolvedValue({ Ok: null }),
+} as unknown as ActorSubclass<_SERVICE>;
+
+test("gets anchor info from actor", async () => {


While it is good to test things, I'm a bit confused as to how getAnchorInfo relates to the identity metadata functionality added in this PR?

I was trying that I could successfully test AuthenticatedConnection and then I thought it might be ok to leave it.

But let's do that in another PR if I want to add more tests. I removed it from this PR.

frederikrothenberger · 2024-06-28T11:38:04Z

src/frontend/src/repositories/anchorMetadata.ts

+  private rawMetadata: RawMetadataState;
+  // Flag to keep track whether we need to commit the metadata to the canister.
+  private updatedMetadata: boolean;
+  private getter: MetadataGetter;


These should be readonly?

frederikrothenberger · 2024-06-28T11:49:58Z

src/frontend/src/repositories/anchorMetadata.ts

+   */
+  getMetadata = async (): Promise<AnchorMetadata | undefined> => {
+    if (!this.metadataIsLoaded(this.rawMetadata)) {
+      await this.loadMetadata();


Shouldn't we simply await this.rawMetadata here? Otherwise you might load things twice / are actually implementing an implicit retry?

Given it's not critical, we should probably avoid that, no?

As agreed, there won't be a loadMetadata call again and this function just returns the pretty metadata if available.

frederikrothenberger · 2024-06-28T11:50:40Z

src/frontend/src/repositories/anchorMetadata.test.ts

+  expect(await instance.getMetadata()).toEqual(mockAnchorMetadata);
+});
+
+test("AnchorMetadataRepository loads data again on init in the background", async () => {


See comment below, but I think the metadata should not be loaded again.

src/frontend/src/repositories/anchorMetadata.test.ts

frederikrothenberger · 2024-06-28T11:53:57Z

src/frontend/src/repositories/anchorMetadata.test.ts

@@ -0,0 +1,223 @@
+import { MetadataMapV2 } from "$generated/internet_identity_types";


I think it would be good to have a test to ensure that the repository leaves additional data unchanged in rawMetadata.

I had a test for this: "IdentityMetadataRepository commits additional metadata to canister after update".

I rephrased the description to use "additional", maybe that was the confusing part.

frederikrothenberger · 2024-06-28T11:57:07Z

src/frontend/src/utils/iiConnection.ts

+    return this.metadataRepository.getMetadata();
+  };
+
+  setPartialMetadata = async (


Suggested change

setPartialMetadata = async (

updateIdentityMetadata = async (

lmuntaner · 2024-07-01T09:45:27Z

@frederikrothenberger ready for another review

lmuntaner · 2024-07-01T10:22:32Z

@frederikrothenberger wait a bit, I found a bug.

lmuntaner · 2024-07-01T10:24:28Z

@frederikrothenberger ready. I forgot to await for the rawMetadata.

frederikrothenberger

Thanks, lgtm! I left one suggestion but I don't feel strongly about it.

frederikrothenberger · 2024-07-02T07:27:25Z

src/frontend/src/repositories/identityMetadata.ts

+    this.rawMetadata = "loading";
+    try {
+      this.updatedMetadata = false;
+      this.rawMetadata = await this.getter();


You could also just put the Promise as a field. That way you could await it alongside a timer with Promise.race(...) in getMetadata, no?

Not sure about the error handling though.

Yes, if I put the promise there I lose the states and I don't see the advantage. Unless I don't put the promise of the getter directly, but I wrap the getter so that it returns a promise of the states.

What do you think it's the advantage? I can do it in another PR. We would also lose the possibility of a client waiting on loadMetadata.

lmuntaner added 2 commits June 27, 2024 15:55

AnchorMetadataRepository

46c7f0b

Improvements

302377b

lmuntaner requested a review from frederikrothenberger June 27, 2024 14:12

Integrate in AuthenticatedConnection

1718335

frederikrothenberger reviewed Jun 27, 2024

View reviewed changes

Add tests for anchor metadata repository and AuthenticatedConnection

0a230a0

lmuntaner commented Jun 28, 2024

View reviewed changes

lmuntaner marked this pull request as ready for review June 28, 2024 10:53

lmuntaner requested a review from a team as a code owner June 28, 2024 10:53

frederikrothenberger reviewed Jun 28, 2024

View reviewed changes

lmuntaner changed the title ~~Anchor Metadata Repository setup~~ Identity Metadata Repository setup Jul 1, 2024

lmuntaner added 2 commits July 1, 2024 11:35

Rename IdentityMetadataRepository

270c219

CR changes

84c55ec

lmuntaner requested a review from frederikrothenberger July 1, 2024 09:43

Rename

982b4a3

Change return

a646146

await metadata

a1930e1

lmuntaner added 2 commits July 1, 2024 12:35

Set metadata if empty

9b28f4b

Update waits for metadata and clean tests

81ef6c6

frederikrothenberger approved these changes Jul 2, 2024

View reviewed changes

lmuntaner added this pull request to the merge queue Jul 2, 2024

Merged via the queue into main with commit 5a685b0 Jul 2, 2024
65 checks passed

lmuntaner deleted the lm-metadata-repository branch July 2, 2024 09:37

lmuntaner mentioned this pull request Jul 3, 2024

Do not show recovery page everytime #2517

Closed

		@@ -0,0 +1,223 @@
		import { MetadataMapV2 } from "$generated/internet_identity_types";

	setPartialMetadata = async (
	updateIdentityMetadata = async (

Identity Metadata Repository setup #2520

Identity Metadata Repository setup #2520

Conversation

lmuntaner commented Jun 27, 2024 • edited by github-actions bot Loading

lmuntaner commented Jun 27, 2024

frederikrothenberger 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

Choose a reason for hiding this comment

lmuntaner commented Jun 27, 2024

frederikrothenberger commented Jun 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmuntaner commented Jun 28, 2024

frederikrothenberger 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

Choose a reason for hiding this comment

lmuntaner Jul 1, 2024 • edited Loading

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

lmuntaner commented Jul 1, 2024

lmuntaner commented Jul 1, 2024

lmuntaner commented Jul 1, 2024

frederikrothenberger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmuntaner commented Jun 27, 2024 •

edited by github-actions bot

Loading

lmuntaner Jul 1, 2024 •

edited

Loading