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: extend agent with a route provider feature #484

Conversation

nikolay-komarevskiy
Copy link
Contributor

@nikolay-komarevskiy nikolay-komarevskiy commented Nov 1, 2023

Description

API Boundary Node (BN) Discovery Library (which is in a very recent development) is the prerequisite for the IC-720: BN Decentralization Feature. This library will have two main responsibilities:

  1. Discovery of the API Boundary Nodes (domains). Users (clients) of the IC need to know, which domains to use when "talking" to the IC. Currently, there is only one domain ic0.app owned by Dfinity. However, once BNs are decentralized, each BN will have it's own/unique domain. Moreover, BNs and their domain records could be added (or removed) from the IC registry via nns proposals. Thus, clients would need an api to discover, which domains currently belong to the IC ecosystem. Note: the exact BN domain discovery mechanism is still being discussed. This could either be accomplished via read_state calls or by fetching records from the registry canister.
  2. Routing - dynamically providing url (BN domain) on demand

Future usages of the library:

  • browsers
  • http gateway
  • icx proxy

The library is being developed within the ic repo and it uses ic-agent as a dependency. However, in the near future, ic-agent could/should include some functionalities of the library or even library as a whole.

This pull request kicks-off the first migration/integration step of the library with the ic-agent.
Namely, ic-agent is extended with a custom RouteProvider feature.

How Has This Been Tested?

  • Existing tests should fully cover the e2e functionality of the RoundRobinRouteProvider which is now used as default route provider for the agent.
  • In addition, several unit tests were added for the RoundRobinRouteProvider.

Checklist:

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

@nikolay-komarevskiy nikolay-komarevskiy force-pushed the extend-agent-with-route-provider branch 4 times, most recently from baa28ca to eeccd23 Compare November 1, 2023 22:42
@nikolay-komarevskiy nikolay-komarevskiy force-pushed the extend-agent-with-route-provider branch from eeccd23 to ae293c5 Compare November 2, 2023 07:52
@nikolay-komarevskiy nikolay-komarevskiy changed the title add RouteProvider Extend agent with a route provider feature Nov 2, 2023
@nikolay-komarevskiy nikolay-komarevskiy changed the title Extend agent with a route provider feature (feat)extend agent with a route provider feature Nov 2, 2023
@nikolay-komarevskiy nikolay-komarevskiy changed the title (feat)extend agent with a route provider feature feat: extend agent with a route provider feature Nov 2, 2023
@nikolay-komarevskiy nikolay-komarevskiy marked this pull request as ready for review November 2, 2023 10:21
@nikolay-komarevskiy nikolay-komarevskiy requested a review from a team as a code owner November 2, 2023 10:21
Copy link
Contributor

@adamspofford-dfinity adamspofford-dfinity left a comment

Choose a reason for hiding this comment

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

I am hesitant to introduce yet another context-free trait object into the agent. Is there any reason a user would configure anything other than a round-robin strategy for a particular list of URLs? If not, this produces much less complexity with the user just providing a list of URLs, and eliminating the trait.

@nikolay-komarevskiy
Copy link
Contributor Author

nikolay-komarevskiy commented Nov 2, 2023

I am hesitant to introduce yet another context-free trait object into the agent. Is there any reason a user would configure anything other than a round-robin strategy for a particular list of URLs? If not, this produces much less complexity with the user just providing a list of URLs, and eliminating the trait.

@adamspofford-dfinity I understand the concern. Imho, RouteProvider should allow for multiple implementations due to the following reasons. Route providers behavior might be configurable based on: latency, geo-location. Also periodic refreshing of the routes list (domains) from the registry could take place, as some API BNs can be added/removed. For round-robin and other route providers, it might also make sense to evict a route, if it fails frequently. Does this answer the question?

Copy link
Contributor

@adamspofford-dfinity adamspofford-dfinity left a comment

Choose a reason for hiding this comment

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

Can you also add an entry to the changelog? Also I mistyped the localhost one, it should be http not https.

@adamspofford-dfinity adamspofford-dfinity merged commit 65b2708 into dfinity:main Nov 6, 2023
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.

2 participants