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

Streamline config shapes (rust) #2215

Closed
21 tasks done
nambrot opened this issue May 10, 2023 · 7 comments · Fixed by #2659
Closed
21 tasks done

Streamline config shapes (rust) #2215

nambrot opened this issue May 10, 2023 · 7 comments · Fixed by #2659

Comments

@nambrot
Copy link
Contributor

nambrot commented May 10, 2023

This is the sister ticket to #2214 . It streamlines the existing chain config with the typescript one as they are so close to each other already. In addition to that streamlining benefit, it also captures the following benefits:

  • It would specify a default RPC for the agent config (similar to SDK-specified default RPCs in ChainMetadata), which would potentially reduce one more argument to need to be specified via ENV vars to get started
  • It moves the rpcConsensusType apart from connection and makes it easier to specify different defaults per agent type (i.e. fallback for relayer and scraper, quorum for validator) and thus obsoletes https://github.com/hyperlane-xyz/issues/issues/438

Subissues

  1. agent good first issue
    mattiekat
  2. agent bug config tech-debt
    mattiekat
  3. agent bug config tech-debt
    mattiekat

Tasks

PRs

@jmrossy
Copy link
Contributor

jmrossy commented Jul 10, 2023

See #2491 for the Typescript-level changes of this workstream

@mattiekat mattiekat self-assigned this Aug 7, 2023
mattiekat added a commit that referenced this issue Aug 11, 2023
### Description

When I was working on #2215 I noticed that we are not using either
zipkin or jaeger at all and possibly never have. Ripping these out to
reduce dead code and also simplify our package dependencies.

### Drive-by changes

None

### Related issues

Discovered through #2215

### Backward compatibility

Yes - we were not using these features and did not document their
existence

### Testing

Manual - Verified that when running deploy-agents, the generated configs
do not have jaeger or zipkin enabled on `main` and then tested to make
sure they still work in this branch
@mattiekat
Copy link
Contributor

Relates to #1865

@mattiekat
Copy link
Contributor

Draft PR to track progress
#2659

mattiekat added a commit that referenced this issue Aug 16, 2023
### Description

Updates the typescript definitions based on the rust code. This is the
first in a series of PRs for handling Rust config streamlining.

### Drive-by changes

- A couple of the core types I refined, please validate the definitions.

### Related issues

Progress on #2215 
Relates to #2491

### Backward compatibility

Yes

### Testing

Unit Tests
mattiekat added a commit that referenced this issue Aug 18, 2023
### Description

This is a fix for an issue in the arguments and environment where we are
unable to correctly support attribute names due to the casing. The main
issue cannot be fully fixed practically by this PR because it would
cause some configuration cases to break. For now it adds manual
exceptions for the command line argument case and leaves envs as they
were. There are then two new parsers that can be used with the new
config format.

### Drive-by changes

Also adds this to argument parser since it was trivial to copy/paste it
and it will allow us to harden parsing when we switch over.

### Related issues

- Fixes #2662 
- Fixes #2663 
- Progress on #2215 

### Backward compatibility

Yes

### Testing

Unit Tests
@mattiekat
Copy link
Contributor

High-level rollout plan proposals:

  1. Support only the new config with the next agent release which would allow us to delete a bunch of dead code now rather than having to do it later and reduces the surface area for bugs
  2. Support both configs by attempting to first parse it as a new config and then parse it as the old config format and create an issue to cleanup deprecated code in the future. Main risk here is that it might make the errors hard to understand if we are not careful.
  3. Support both configs using a flag to indicate the which config should be read. This would allow us silently roll out the changes internally and eventually cut over by changing the default to the new config format. Downside is it means defining a new flag either as env or arg and it also still has the deprecated code hanging around.
  4. Something else?

(1) is the easiest but (3) is pretty easy. (2) might be finicky.

mattiekat added a commit that referenced this issue Aug 18, 2023
### Description

This is a big one for #2215...

1. Splits the parsing of configs from the config definitions to support
both the old, new, and future parsers simultaneously. (We might want to
rename it from parser to config format or something; not set on this
name).
2. Implements the new config format without switching to it or enabling
it. This means this PR does introduce dead code that is going to be used
in the next PR.

### Drive-by changes

- Deleted some unused (or single use) macros defined in hyperlane base
- Updated the definition of the config parsing trait to simplify
implementation
- Added some more ergonomic methods of handling errors in config parsing
- Removed any raw `unwrap()` I noticed since these cause difficult to
debug errors in prod.
- Added some additional validation to catch code mistakes in config path
definitions

### Related issues

- Progress on #2215
- Comes after #2658

### Backward compatibility

Yes

### Testing
Manual (E2E only for now to make sure I did not break the old config
format)
mattiekat added a commit that referenced this issue Aug 29, 2023
### Description

This does two things:
1) It parses the agent-specific configs for the relayer, validator, and
scraper.
2) It removes the rigid structures that had been in place for parsing
that were called out earlier and instead parses the serde json values
directly.

This PR does not cut over to the new versions just yet. That can happen
once we start integrating it into the testing and deployment pipelines.

### Drive-by changes

- Derive more was added and used where it made sense
- The old config macro was removed

### Related issues

- Progress on #2215 

### Backward compatibility

Yes

### Testing

None
@mattiekat
Copy link
Contributor

This will be kept up to date with main, and then after v3 is merged, we will go ahead and merge this in before we release.

@avious00
Copy link
Contributor

@tkporter to review helm charts portion, dan has done his review

@nambrot
Copy link
Contributor Author

nambrot commented Sep 21, 2023

@mattiecnvr is #2659 the PR that we will merge into v3?

mattiekat added a commit that referenced this issue Oct 2, 2023
### Description
Continues the updates to the rust config shapes by updating deployment
and runtime expectations. This PR also attempts to rely on the new
single source of truth created by the schema in the SDK as much as
reasonably possible which helped delete more code but also give some
guarantees of consistency.

THIS IS A BREAKING CHANGE! It changes the config shapes the agents want
and we should not merge this until we are ready.

Fixes #2215

---------

Co-authored-by: Guillaume Bouvignies <guillaumebouvignies@gmail.com>
Co-authored-by: Yorke Rhodes <yorke@hyperlane.xyz>
Co-authored-by: Guillaume Bouvignies <guillaume.bouvignies@kurtosistech.com>
@avious00 avious00 closed this as completed Oct 3, 2023
yorhodes added a commit that referenced this issue Oct 3, 2023
Continues the updates to the rust config shapes by updating deployment
and runtime expectations. This PR also attempts to rely on the new
single source of truth created by the schema in the SDK as much as
reasonably possible which helped delete more code but also give some
guarantees of consistency.

THIS IS A BREAKING CHANGE! It changes the config shapes the agents want
and we should not merge this until we are ready.

Fixes #2215

---------

Co-authored-by: Guillaume Bouvignies <guillaumebouvignies@gmail.com>
Co-authored-by: Yorke Rhodes <yorke@hyperlane.xyz>
Co-authored-by: Guillaume Bouvignies <guillaume.bouvignies@kurtosistech.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants