Remove secret_connection and ring dependency

[yihuang]

I just upgraded the ring dependency, changed secret_connection.rs accordingly, hope you like it.
We are using tendermint-rs in our project, there are probably a few PRs following this one ;D

[tarcieri]

Note that per #27 I thought the plan was to move Secret Connection back to the KMS repo (in which case the Secret Connection impl in this crate should just be deleted).

Are you actually using Secret Connection via this crate?

In the KMS repo, I moved the (re-vendored) Secret Connection implementation from ring over to the chacha20poly1305 crate to avoid future ring-asm conflicts (#11):

tendermint/tmkms#366

[yihuang]

Good to know that, we don't use secret_connection. We upgrade this only because the version of ring conflicts with our dependencies. I would love to see it removed ;D

[tarcieri]

@yihuang are you experiencing that conflict even when the secret-connection Cargo feature is disabled? I would've thought that would avoid it, and that's what KMS is presently doing:

https://github.com/tendermint/kms/blob/master/Cargo.toml#L49

But yeah, ring-asm conflicts are super annoying, which is why I'd like to get rid of ring, especially considering we only use it for ChaCha20Poly1305.

[yihuang]

I don't see optional dependencies in git master version of tendermint-rs, I was not looking at the most recent version?

[tarcieri]

@yihuang all of the cargo features were removed in the git master (with the goal of splitting things up into different crates)

The latest release (sans a point release to re-export signatory) is at:

https://github.com/interchainio/tendermint-rs/tree/v0.10.0/tendermint-rs

secret-connection is opt-in, so I'd be surprise if you saw ring conflicts unless you specifically activated it.

[yihuang]

Thanks, I see. But I think we will stick to the git version anyway because we want to work on the lite client implementation.

[tarcieri]

Aha! Ok

There seems to be a bit of confusion around #27, but I think the plan is still to continue Secret Connection work in the KMS repo, in which case I'd suggest deleting the implementation in this repo.

[yihuang]

So I just changed the commit to remove secret_connection and related dependencies(hope I've got them all).

[tarcieri]

If the tests pass, this looks good to me.

@ebuchman looks like CI may not be running?

[liamsi]

looks like CI may not be running?

Looks like all PRs coming from forks (not branches in this repo) do not trigger CI.

[tarcieri]

@liamsi weird, wonder why #66 is green. It'd be nice to merge this one.

[liamsi]

🤔 I think #66 is green because it seems CI is not required for this branch (interchainio:lite_impl_simple_merkle_merged)? Also noteworthy: I could merge #36 even without an approving review. (master seems to require an approving review and CI in case it comes from a branch). I suspect that if you approve this PR it can be merged; I guess CI would then run on merge.

For master GPG signed commits also seem to be a requirement.

[yihuang]

Do I need to re-commit with GPG signing on?

[tarcieri]

@yihuang looks like it. can you give that a try?

[liamsi]

I agree, it could unblock merging this (hopefully). I think I've already merged a few of @yihuang's PRs into my branch (apparently GPG signed commits aren't required there). Hope we won't have to redo those too 🤞

[yihuang]

Re-commited with pgp signing, also removed tendermint/src/amino_types/secret_connection.rs.

[tarcieri]

Still saying it's waiting for GPG status to be reported 😢

@ebuchman any idea what's up with the GPG check? It seems to be rejecting GPG signed commits.

Also, are you sure you really want GPG signed commits? (as opposed to e.g. GPG signed tag objects for releases). See:

https://mikegerwitz.com/2012/05/a-git-horror-story-repository-integrity-with-signed-commits

[liamsi]

I'd also vote for signing releases / tags only.

[liamsi]

Not sure what is going on here. Now all commits are signed but GPG still shows up as "expected". Similarly for #59.

[liamsi]

Thanks @greg-szabo!

[liamsi]

OK, CI doesn't run on forks but the GPG issue is resolved. Will merge now. Thanks again @yihuang :-)