hyperlane-xyz / hyperlane-monorepo

The home for Hyperlane core contracts, sdk packages, and other infrastructure
https://hyperlane.xyz
Other
311 stars 341 forks source link

Retrying provider should be configurable #2408

Open nambrot opened 1 year ago

nambrot commented 1 year ago

Agents wrap a classic RPC provider in a RetryingProvider to protect against intermittent RPC issues.

However, that is currently not configurable and some operators may want to retry less often (or just differently).

While at it, the defaults should probably be adjusted, no need to retry 6 times, 2 or 3 seems sufficient

Ideally, the configuration is somewhat consistent with the retry configuration of chainmetadata for the typescript providers that we have specced in https://github.com/hyperlane-xyz/hyperlane-monorepo/pull/2203/files#diff-0dda34e51b45fcb259676791b216443495e465f6ec7c372fc77ec9abe9b8409aR155

NOOMA-42 commented 9 months ago

Hi I'm interested in looking into this issue. I'd like to clarify where should I put the config file? the ts config is in SDK. Should I put the rust config in https://github.com/hyperlane-xyz/hyperlane-monorepo/blob/main/rust/chains/hyperlane-ethereum/src/config.rs ? or config folder for rust in general: https://github.com/hyperlane-xyz/hyperlane-monorepo/tree/main/rust/config

nambrot commented 9 months ago

I'd recommend to check out the rust config scheme right now. The base settings struct is here https://github.com/hyperlane-xyz/hyperlane-monorepo/blob/ece2be507a8a3852f9496d06ae2c53939da9e874/rust/hyperlane-base/src/settings/base.rs#L43

NOOMA-42 commented 8 months ago

I've reviewed hyperlane-base, hyperlane-core, and hyperlane-ethereum. Based on your suggestion, I'm considering implementing the retry setting in hyperlane-base. However, I'm currently stuck at two issues:

  1. It appears that hyperlane-base relies on hyperlane-ethereum. For instance, in hyperlane-base, the following dependency is noted: https://github.com/hyperlane-xyz/hyperlane-monorepo/blob/f3b7ddb699fda050af7c90bd34ad91c4f6864021/rust/hyperlane-base/src/settings/signers.rs#L66 If that's the case and config still need to implement in hyperlane-base and imported in hyperlane-ethereum, there will be a cyclic dependency. This observation also leads me to question the documentation which states that hyperlane-ethereum depends on hyperlane-base. Could this be a slight discrepancy? https://github.com/hyperlane-xyz/hyperlane-monorepo/blob/f3b7ddb699fda050af7c90bd34ad91c4f6864021/rust/README.md?plain=1#L183

  2. Regarding the placement of the configuration in hyperlane-base, I need some clarification. According to the documentation, hyperlane-base is intended for agents, but it's unclear if it also applies to providers. https://github.com/hyperlane-xyz/hyperlane-monorepo/blob/f3b7ddb699fda050af7c90bd34ad91c4f6864021/rust/README.md?plain=1#L170

I'd recommend to check out the rust config scheme right now. The base settings struct is here

https://github.com/hyperlane-xyz/hyperlane-monorepo/blob/ece2be507a8a3852f9496d06ae2c53939da9e874/rust/hyperlane-base/src/settings/base.rs#L43

nambrot commented 8 months ago

@tkporter and @daniel-savu are more familiar with the rust code base than I am

daniel-savu commented 8 months ago

Hey @NOOMA-42, thanks for taking on this issue.

Indeed those docs are outdated - feel free to update as you proceed with this task. The description of crate responsibilities is accurate, but the order of dependencies is:

hyperlane-core <---(depended on by)--- hyperlane-{chainname} <---(depended on by)--- hyperlane-base <---(depended on by)--- agents

Always check the source of truth when in doubt (cargo.toml in this case).

Regarding the placement of the configuration in hyperlane-base, I need some clarification. According to the documentation, hyperlane-base is intended for agents, but it's unclear if it also applies to providers.

Providers are defined by the chain-specific hyperlane-* crates and instantiated in hyperlane-base, so it does seem to me like a good place to add it. More specifically, I'd turn this enum into a struct with the same name that stores the chain-specific settings enum as a field, but now also has a new field retrying_provider_interval: Option<u64> or similar.

nambrot commented 7 months ago

@tkporter didn't you add some more configurability as part of indexing?

tkporter commented 6 months ago

I view this pretty separate from indexing as it's used for any kind of RPC. We'll be adding some more configurability as part of indexing but it's focused on indexing-specific things