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!: add support for fetching root keys automatically #569

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

rupansh
Copy link

@rupansh rupansh commented Jul 14, 2024

Description

This change introduces a way to fetch root keys automatically on demand. This makes the construction of the agent a bit more ergonomic for local nodes.
Minor breakage to the API had to be introduced unfortunately. I also recommend deprecating the fetch_root_keys method.

How Has This Been Tested?

I ran cargo test. Further, this version will also be used by the Yral fronend for testing our changes locally (https://github.com/go-bazzinga/hot-or-not-web-leptos-ssr/blob/rupansh/testcontainers/src/state/canisters.rs#L30)

Checklist:

  • The title of this PR complies with Conventional Commits.
  • I have edited the CHANGELOG accordingly.
  • I have made corresponding changes to the documentation.

@rupansh rupansh requested a review from a team as a code owner July 14, 2024 22:19
@sa-github-api
Copy link

Dear @rupansh,

In order to potentially merge your code in this open-source repository and therefore proceed with your contribution, we need to have your approval on DFINITY's CLA.

If you decide to agree with it, please visit this issue and read the instructions there. Once you have signed it, re-trigger the workflow on this PR to see if your code can be merged.

— The DFINITY Foundation

@rupansh rupansh closed this Jul 14, 2024
@rupansh rupansh reopened this Jul 14, 2024
@rupansh rupansh changed the title feat: add support for fetching root keys automatically feat!: add support for fetching root keys automatically Jul 14, 2024
@adamspofford-dfinity
Copy link
Contributor

Why is this needed vs just calling fetch_root_key immediately after constructing the agent?

@rupansh
Copy link
Author

rupansh commented Jul 15, 2024

Why is this needed vs just calling fetch_root_key immediately after constructing the agent?

This proposes a more ergonomic API,
fetch_root_key is a part of construction of the agent,
currently this becomes a 2 part process

  1. You first configure the agent through the builder
  2. You have to further configure the agent through a second call to fetch_root_key.

Other than the ergonomic benefits, the construction of the agent becomes synchronous instead of requiring async.
This is useful if you need a global static agent, or you are in an environment where initializing the agent asynchronously might not be trivial (we encountered this while using the agent in leptos for example)

Imagine a global static agent,
before every call to the replica, one would have to ensure fetch_root_key is called, pervasing the call throughout the code base.

) -> Result<Vec<u8>, AgentError> {
match delegation {
None => Ok(self.read_root_key()),
None => Ok(root_key),

Choose a reason for hiding this comment

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

What is the reason for passing root_key as a parameter here, rather than getting it from self?

Copy link
Author

Choose a reason for hiding this comment

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

verify_cert & check_delegation would become async and you have a recursive future. I wasn't able to unroll it into an iterator unfortunately, so I went with this

@adamspofford-dfinity
Copy link
Contributor

Other than the ergonomic benefits, the construction of the agent becomes synchronous instead of requiring async.
This is useful if you need a global static agent

Only for lazy-init statics (e.g. LazyLock), yes? Init-once statics (e.g. OnceLock) have no trouble being async. And for an agent which may or may not be pointed at the local replica, doesn't this already lend itself to init-once style due to the replica being used being a runtime parameter?

we encountered this while using the agent in leptos for example

Do you have an example of this?

@rupansh
Copy link
Author

rupansh commented Jul 16, 2024

Other than the ergonomic benefits, the construction of the agent becomes synchronous instead of requiring async.
This is useful if you need a global static agent

Only for lazy-init statics (e.g. LazyLock), yes? Init-once statics (e.g. OnceLock) have no trouble being async. And for an agent which may or may not be pointed at the local replica, doesn't this already lend itself to init-once style due to the replica being used being a runtime parameter?

Indeed

we encountered this while using the agent in leptos for example

Do you have an example of this?

Sure, most of the code is written in a dual-environment style in leptos(i.e targeting both browser and server):
We are providing a global agent here: https://github.com/go-bazzinga/hot-or-not-web-leptos-ssr/blob/rupansh/testcontainers/src/app.rs#L65

A way to force async initialization would be by using create_blocking_resource, but these run server side and need the result to be Serializable (which agent is not)
Technically I can send the root key server side as a workaround but these are further reduced ergonomics, and introduces latency to the initial load of the website

I understand that this might be too specific to our use case which is why my main emphasis is on the ergonomics rather than better global statics

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants