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

[Documentation]: Documentation for adding Rosetta support for 0.47 is wrong #17608

Closed
fmorency opened this issue Sep 1, 2023 · 4 comments
Closed
Labels
T:Docs Changes and features related to documentation.

Comments

@fmorency
Copy link
Contributor

fmorency commented Sep 1, 2023

Summary

The Rosetta documentation for 0.47 found in 1 is wrong on chains scaffolded by Ignite. I'm uncertain about the correct fix.

The below describes what I did to make it work.

  1. The import
    import "github.com/cosmos/cosmos-sdk/server"
    should be replaced by
    import rosettaCmd "cosmossdk.io/tools/rosetta/cmd"
  2. The AddCommand should be located at the end of the initCmd function to mimic simapp
  3. The AddCommand
    rootCmd.AddCommand(
      server.RosettaCommand(encodingConfig.InterfaceRegistry, encodingConfig.Codec)
    )
    should be replaced by
    rootCmd.AddCommand(rosettaCmd.RosettaCommand(encodingConfig.InterfaceRegistry, encodingConfig.Marshaler))

I'm uncertain where the discrepancy between encodingConfig.Codec and encodingConfig.Marshaler comes from, as both are correct in their respective repositories.

I can create a PR if someone points me in the right direction for this fix.

@fmorency fmorency added the T:Docs Changes and features related to documentation. label Sep 1, 2023
@julienrbrt
Copy link
Member

Feel free to open a PR targeting the release/v0.47.x branch. Happy to review!
However, encodingConfig.Codec is still right, see https://github.com/cosmos/cosmos-sdk/blob/v0.47.5/simapp/simd/cmd/root.go#L189

@fmorency
Copy link
Contributor Author

fmorency commented Sep 1, 2023

Feel free to open a PR targeting the release/v0.47.x branch. Happy to review! However, encodingConfig.Codec is still right, see https://github.com/cosmos/cosmos-sdk/blob/v0.47.5/simapp/simd/cmd/root.go#L189

Will do, thanks.

Yes, it is still correct in the context of the simapp but NOT in the context of chains scaffolded by Ignite.

type EncodingConfig struct {
	InterfaceRegistry types.InterfaceRegistry
	Marshaler         codec.Codec
	TxConfig          client.TxConfig
	Amino             *codec.LegacyAmino
}

Thoughts?

@julienrbrt
Copy link
Member

Feel free to open a PR targeting the release/v0.47.x branch. Happy to review! However, encodingConfig.Codec is still right, see v0.47.5/simapp/simd/cmd/root.go#L189

Will do, thanks.

Yes, it is still correct in the context of the simapp but NOT in the context of chains scaffolded by Ignite.

type EncodingConfig struct {
	InterfaceRegistry types.InterfaceRegistry
	Marshaler         codec.Codec
	TxConfig          client.TxConfig
	Amino             *codec.LegacyAmino
}

Thoughts?

Docs from the SDK follow the SDK patterns, so we should not mind the fact that Ignite names the variable differently.

@fmorency
Copy link
Contributor Author

fmorency commented Sep 1, 2023

I was about to disagree because Ignite was recommended by CosmosSDK... but I just stumbled on #15521.

I will prepare something against the v0.47.x branch and stick to SDK patterns. Thanks @julienrbrt!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T:Docs Changes and features related to documentation.
Projects
None yet
Development

No branches or pull requests

2 participants