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

Publish workspace to crates.io #4978

Open
conorsch opened this issue Jan 10, 2025 · 3 comments
Open

Publish workspace to crates.io #4978

conorsch opened this issue Jan 10, 2025 · 3 comments
Assignees

Comments

@conorsch
Copy link
Contributor

Overview

We want to publish the crates in the protocol repo workspace to https://crates.io, so that external developers can depend on Penumbra code when writing downstream tooling. To date, we've recommended that external developers use git dependendencies in Rust projects, e.g.

penumbra-transaction = { git = "https://github.com/penumbra-zone/penumbra", rev = "ac7abacc9bb09503d6fd6a396bc0b6850079084e", features = [
   download-proving-keys",
] }

but doing so is not possible for projects already publishing their crates to crates.io. Two notable projects we intend to support are Astria (#3125) and Hermes. We're already maintaining a fork of Hermes, and publishing the crates would unblock us from submitting the Penumbra integration for Hermes to upstream.

Work to date

There's currently some WIP around the workspace consolidation happening in the following branches:

That work should be appended to, and organized in a feature branch that can be PR'd into main. In the process, we'll likely attempt to resolve both #4400 & #4682.

@conorsch conorsch self-assigned this Jan 10, 2025
@github-actions github-actions bot added the needs-refinement unclear, incomplete, or stub issue that needs work label Jan 10, 2025
@conorsch conorsch removed the needs-refinement unclear, incomplete, or stub issue that needs work label Jan 10, 2025
@erwanor
Copy link
Member

erwanor commented Jan 10, 2025

Thanks for writing this up and tracking it, fwiw:

In the process, we'll likely attempt to resolve both #4400 & #4682.

I think this is already done in v0.82.x branch. v1 reflection was only released as part of [email protected] iirc.

The release branch should have all of the ecosystem bumps (tonic, ibc-types, tendermint, tower, etc.)

@conorsch
Copy link
Contributor Author

conorsch commented Jan 10, 2025

The release branch should have all of the ecosystem bumps (tonic, ibc-types, tendermint, tower, etc.)

That's true, the release branch does contain a commit that bumps the deps (#4973), but unfortunately it contains regressions like breaking HTTPS support for tonic clients. Fixing that regression is a bit of a side quest from the described goal of publishing the workspace, but worthwhile to get things straightened out. I'll follow up with PRs into main that describe the problem in detail.

erwanor pushed a commit that referenced this issue Jan 11, 2025
## Describe your changes

This test is off-by-default, given that it talks to a remote endpoint,
and is rather slow. It can be run directly via:

cargo nextest run --release -p pcli --features integration-testnet
sync_wallet_on_public_testnet

As usual, make sure to include the `--release` flag, otherwise it'll be
much slower.

## Issue ticket number and link

Related to changes described in #4978. Specifically, there were changes
made in #4973 that broke ClientTls configs for tonic connections, but
our test suite didn't catch that.

## Testing and review

1. Check this branch out locally and run `cargo nextest run --release -p
pcli --features integration-testnet sync_wallet_on_public_testnet`
2. Confirm that test passes!

It's also worth considering clicking this on in CI so we get assurance;
I left it off by default but plan to ask for it to be used in subsequent
testing towards #4978. Maybe we should just stick it in CI, at least
temporarily?

## Checklist before requesting a review

- [x] I have added guiding text to explain how a reviewer should test
these changes.

- [x] If this code contains consensus-breaking changes, I have added the
"consensus-breaking" label. Otherwise, I declare my belief that there
are not consensus-breaking changes, for the following reason:

> Test-only, no changes to app code. Intended to reduce the odds of
breaking changes being merged.
@erwanor
Copy link
Member

erwanor commented Jan 12, 2025

Thanks for the write-up, my understanding is that this is caused by the rustls backend change to aws-lc-rs (and away from ring). I think this might potentially cause platform issues down the road, but we'll figure it out when we get there (and hopefully, the ecosystem will beat us to it).

  1. cargo add aws-lc-rs
  2. let _ = aws_lc_rs::default_provider().install_default(); per process (so, e.g, one in `main.rs in the pcli bin)
  3. Tracking the tls_config calls to make sure that they configured with the bundled certificates: tls_config(ClientTlsConfig::new().with_webpki_roots())?

I am not entirely sure that this is sufficient to unblock, but that's what I got from hacking on this a bit.

conorsch added a commit that referenced this issue Jan 14, 2025
## Describe your changes
This PR re-implements the changes from #4973, which were merged into the
`release/v0.82.x` branch, but never landed on main. I'm resubmitting
them so that we can address the HTTPS breakage, documented below, prior
to tackling the rest of the changes required for getting the workspace
crates published (#4978).

Continuation of #4963, into a release branch `v0.82.x` before tagging a
release candidate at that version and publishing the workspace using an
`alpha` version.

This handles the domain type change for upgradeable channels
(penumbra-zone/ibc-types#84) smoothly. It makes
sure to write default values to the new fields, which avoids wire
protocol changes, and makes this PR non consensus/state breaking.

Includes substantial version changes to:

* tendermint-rs
* tonic #4400
* ibc-types #4682
* cnidarium #4956

## Issue ticket number and link

This PR resubmits the changes in #4973, in an attempt to isolate
problematic behavior.

## Testing and review

The primary motivation for this changeset was to address the following
error, which occurred when I tried to sync a wallet against testnet
(using an HTTPS connection):
```
2025-01-10T22:41:38.135876Z DEBUG load_or_initialize{path=Some("/tmp/nix-shell.5VH2AV/tmp.4Ay22mKmyY/pcli-view.sqlite") url=https://testnet.plinfra.net/}: penumbra_view::storage: database does not exist path="/tmp/nix-shell.5VH2AV/tmp.4Ay22mKmyY/pcli-view.sqlite"
thread 'main' panicked at /home/conor/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rustls-0.23.20/src/crypto/mod.rs:249:14:
no process-level CryptoProvider available -- call CryptoProvider::install_default() before this point
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
```

The most recent commit on this branch addresses that problem, by

1. explicitly configuring a default crypto provider via
[rustls](https://docs.rs/rustls)
2. reusing a new `ViewServer::get_pd_channel` method throughout the
codebase to handle conditional TLS config

Feedback welcome on whether the new logic is clearly documented and
stored in the right place.

## Checklist before requesting a review

- [x] I have added guiding text to explain how a reviewer should test
these changes.

- [x] If this code contains consensus-breaking changes, I have added the
"consensus-breaking" label. Otherwise, I declare my belief that there
are not consensus-breaking changes, for the following reason:

> There are significant version bumps in this patch set, but previous
discussion concluded that the changes are not consensus-breaking; see
#4682 (comment)

---------

Co-authored-by: Erwan Or <[email protected]>
Co-authored-by: Richard Janis Goldschmidt <[email protected]>
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

No branches or pull requests

2 participants