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

Remove cosmos/keyring #548

Merged
merged 5 commits into from
Aug 11, 2022
Merged

Conversation

rootulp
Copy link
Collaborator

@rootulp rootulp commented Jul 14, 2022

Resolves #538

cosmos/keyring hasn't pulled upstream changes from 99designs/keyring yet and the latest release of 99designs/keyring (1.2.1) appears to contain a fix for MacOS warnings: 99designs/keyring#107

Links

make test passes after this change and it looks like other Cosmos chains don't need this replace:

Does anyone know if it's still necessary? I couldn't find the justification in #127 which added it.

cosmos/keyring hasn't pulled upstream changes from 99designs/keyring yet
and the latest release of 99designs/keyring (1.2.1) appears to contain
a fix for MacOS warnings: 99designs/keyring#107

Links
- https://github.com/cosmos/keyring/tags
- https://github.com/99designs/keyring/releases/tag/v1.2.1

`make test` passes after this change and it looks like other Cosmos
chains don't need this replace:
- https://github.com/osmosis-labs/osmosis/blob/main/go.mod#L274-L285=
- https://github.com/CosmosContracts/juno/blob/main/go.mod#L178-L182=

Does anyone know if it's still necessary? I couldn't find the justification in
celestiaorg#127 which added it.
go.sum Outdated Show resolved Hide resolved
@adlerjohn
Copy link
Member

cc @evan-forbes

@evan-forbes
Copy link
Member

I don't have a mac to test, but this is plenty good enough for me cosmos/cosmos-sdk#12576

we merge this so that we don't have to wait for the next cosmos-sdk upgrade I think

@rootulp
Copy link
Collaborator Author

rootulp commented Jul 14, 2022

cosmos/cosmos-sdk#12576 merged but I don't think this PR is ready b/c it won't resolve warnings on macOS until we perform a similar replace removal in celestiaorg/cosmos-sdk.

I'd rather not add a direct dependency in celestia-app on keyring v1.2.1 b/c it isn't a true direct dependency of this repo.

Defer to @evan-forbes on our strategy for upgrading celestiaorg/cosmos-sdk. Options I can see:

  1. Wait until a cosmos-sdk release is cut and then pull upstream changes to celestiaorg/cosmos-sdk.
  2. If it isn't a maintenance burden, I can cherry-pick the commit to celestiaorg/cosmos-sdk but I assume we want to minimize the diff between celestiaorg/cosmos-sdk and cosmos/cosmos-sdk.

@evan-forbes
Copy link
Member

Yeah, we can just cherry pick those changes in our fork's v0.46.x-celestia branch (which is currently based off of the v0.46.0-beta2 tag)

@rootulp
Copy link
Collaborator Author

rootulp commented Jul 22, 2022

Blocked on a release of https://github.com/celestiaorg/cosmos-sdk

@evan-forbes
Copy link
Member

I think we can close this as it will be included in the upgrade to v0.46.0 #472, but will defer to @rootulp

@rootulp
Copy link
Collaborator Author

rootulp commented Jul 27, 2022

Based on cosmos/cosmos-sdk#12707 it looks like it will be included in v0.46.0 so agreed, thanks @evan-forbes !

@rootulp rootulp closed this Jul 27, 2022
@rootulp
Copy link
Collaborator Author

rootulp commented Jul 27, 2022

On second thought, I think we still want to remove the replace in this repo

@rootulp rootulp reopened this Jul 27, 2022
@rootulp
Copy link
Collaborator Author

rootulp commented Jul 27, 2022

Status: still blocked on a release of https://github.com/celestiaorg/cosmos-sdk

@celestiaorg celestiaorg deleted a comment from codecov-commenter Jul 27, 2022
@rootulp
Copy link
Collaborator Author

rootulp commented Aug 3, 2022

Release was cut: https://github.com/celestiaorg/cosmos-sdk/releases/tag/v1.2.0-sdk-v0.46.0

Will update this PR after #472 merges

@rootulp rootulp requested a review from evan-forbes August 10, 2022 02:30
@rootulp rootulp self-assigned this Aug 10, 2022
@rootulp rootulp marked this pull request as ready for review August 10, 2022 02:30
@rootulp rootulp added dependencies Pull requests that update a dependency file chore optional label for items that follow the `chore` conventional commit labels Aug 10, 2022
@rootulp rootulp merged commit fb068e2 into celestiaorg:main Aug 11, 2022
@rootulp rootulp deleted the rp/remove-cosmos-keyring branch August 11, 2022 18:21
@rootulp rootulp mentioned this pull request Nov 21, 2023
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore optional label for items that follow the `chore` conventional commit dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

make install results in warning: 'SecKeychainCreate' is deprecated
3 participants