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

feat: await fetchRootKey calls while making API calls #963

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nathanosdev
Copy link
Member

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context.

Fixes # (issue)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration.

Checklist:

  • My changes follow the guidelines in CONTRIBUTING.md.
  • The title of this PR complies with Conventional Commits.
  • I have edited the CHANGELOG accordingly.
  • I have made corresponding changes to the documentation.

move fetchRootKey calls from constructor to API calls

BREAKING CHANGE:
this.#rootKey = options.rootKey ?? fromHex(IC_ROOT_KEY);
this.#shouldFetchRootKey = options.shouldFetchRootKey ?? false;
// kick off the fetchRootKey process asynchronously, if needed
(async () => await this.fetchRootKey())();
Copy link
Member Author

Choose a reason for hiding this comment

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

We start fetching the root key as soon as possible, in practice this should mean that there are no further delays compared to creating the agent with an async constructor and waiting for this task to complete before making API calls.

@@ -245,19 +253,21 @@ other computations so that this class can stay as simple as possible while
allowing extensions.
*/
export class HttpAgent implements Agent {
public rootKey: ArrayBuffer;
Copy link
Member Author

Choose a reason for hiding this comment

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

I've removed the public rootKey property in favor of the async getRootKey, since we cannot guarantee availability of the correct root key when fetchRootKey is set to true.

this.#rootKeyFetched = true;
if (!this.#shouldFetchRootKey) {
return this.#rootKey;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

If we should not fetch the root key, we return the existing root key, which is either a hard coded root key provided by the consumer or the mainnet root key.

}

if (this.#fetchRootKeyPromise === null) {
this.#fetchRootKeyPromise = this.#fetchRootKey();
Copy link
Member Author

@nathanosdev nathanosdev Jan 13, 2025

Choose a reason for hiding this comment

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

If we should fetch the root key, and this is the first time this function is called, we create the promise to fetch the root key. On subsequent calls to this function, the promise will have already been made so we will skip this.

}
return this.rootKey;

return await this.#fetchRootKeyPromise;
Copy link
Member Author

@nathanosdev nathanosdev Jan 13, 2025

Choose a reason for hiding this comment

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

Finally we await the promise and return the resulting root key. If the promise has already resolved, we can just continue reusing the resolved promise.

@nathanosdev nathanosdev marked this pull request as ready for review January 13, 2025 19:53
@nathanosdev nathanosdev requested a review from a team as a code owner January 13, 2025 19:53
@nathanosdev nathanosdev self-assigned this Jan 13, 2025
@nathanosdev nathanosdev requested a review from krpeacock January 13, 2025 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant