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

feat: auto-detect chain-prefix from chain binary #98

Closed
wants to merge 3 commits into from

Conversation

rnbguy
Copy link
Member

@rnbguy rnbguy commented Aug 24, 2022

Closes #97

  • Add tests

@rnbguy rnbguy added the enhancement New feature or request label Aug 24, 2022
@rnbguy rnbguy requested a review from a team August 24, 2022 11:26
@rnbguy rnbguy self-assigned this Aug 24, 2022
@rnbguy rnbguy marked this pull request as ready for review August 29, 2022 18:12
Copy link
Contributor

@andrey-kuprianov andrey-kuprianov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While a nice-to-have feature per se, it is rushed and unready to be merged at this point; it will create more problems than solve:

  • only the prefix is detected, but not denom; this is confusing. The user will have to provide some aspects about the blockchain, but not others. Half automation in this case is actually worse than no automation at all.
  • it is unclear how auto-detections works, and should work in general when the prefix can be provided in the chain.toml config file.
  • the feature relies on the particular way a binary is expected to react to keys parse 00 --output json. I don't feel this is the standard behavior we should expect of all chains.
  • the feature is undocumented. E.g. the transfer tutorial is not adjusted.

First and foremost, it's unclear whether this feature is needed at all from our users. In any case, it is definitely of a low priority. Please postpone working on this feature till after the prototype release, and till we discuss it within the team.

@rnbguy
Copy link
Member Author

rnbguy commented Aug 30, 2022

only the prefix is detected, but not denom; this is confusing. The user will have to provide some aspects about the blockchain, but not others. Half automation in this case is actually worse than no automation at all.

Denom can be configured in genesis file generation step. It is not exactly hardcoded as chain HRP prefixes.

it is unclear how auto-detections works, and should work in general when the prefix can be provided in the chain.toml config file.

We can still keep prefix key in chain.toml and update it when binary is updated (via atomkraft chain binary junod)

the feature relies on the particular way a binary is expected to react to keys parse 00 --output json. I don't feel this is the standard behavior we should expect of all chains.

I agree with the fact that we should not use a feature that is not supported by cosmos-SDK by default. I opened a PR at cosmos-sdk. But I don't expect it to be present in any other chains soon.

the feature is undocumented. E.g. the transfer tutorial is not adjusted.

I wait for an initial code review to add tests and update tutorials. Because it doesn't make sense if a PR is rejected.

@andrey-kuprianov andrey-kuprianov changed the title Auto detect chain-prefix from chain binary [WIP] Auto detect chain-prefix from chain binary Aug 30, 2022
@rnbguy rnbguy changed the title [WIP] Auto detect chain-prefix from chain binary feat: auto-detect chain-prefix from chain binary Nov 11, 2022
@rnbguy rnbguy marked this pull request as draft November 11, 2022 18:12
@hvanz hvanz added urgent and removed urgent labels Nov 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Auto-detect chain prefix from chain binary
4 participants