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

refactor!: eliminate sdk codec dependency from remote mode #107

Merged
merged 8 commits into from
Mar 19, 2024

Conversation

dadamu
Copy link
Contributor

@dadamu dadamu commented Feb 6, 2024

Description

This PR eliminate the codec dependency from remote node to make Juno chain-agnostic.

Checklist

  • Targeted PR against correct branch.
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Wrote unit tests.
  • Re-reviewed Files changed in the Github PR explorer.

@dadamu dadamu force-pushed the paul/SDK-72/eliminate-sdk-codec-in-remote-mode branch from e7064a4 to afd4cf8 Compare February 6, 2024 14:42
@dadamu dadamu marked this pull request as ready for review February 7, 2024 10:34
@dadamu dadamu requested a review from RiccardoM February 7, 2024 10:34
Comment on lines -59 to -89
type EncodingConfigBuilder func() appparams.EncodingConfig

var (
ModuleBasics = module.NewBasicManager(
auth.AppModuleBasic{},
genutil.NewAppModuleBasic(genutiltypes.DefaultMessageValidator),
bank.AppModuleBasic{},
capability.AppModuleBasic{},
staking.AppModuleBasic{},
mint.AppModuleBasic{},
distr.AppModuleBasic{},
gov.NewAppModuleBasic(
[]govclient.ProposalHandler{
paramsclient.ProposalHandler,
upgradeclient.LegacyProposalHandler,
upgradeclient.LegacyCancelProposalHandler,
},
),
params.AppModuleBasic{},
crisis.AppModuleBasic{},
slashing.AppModuleBasic{},
feegrantmodule.AppModuleBasic{},
upgrade.AppModuleBasic{},
evidence.AppModuleBasic{},
authzmodule.AppModuleBasic{},
groupmodule.AppModuleBasic{},
vesting.AppModuleBasic{},
nftmodule.AppModuleBasic{},
consensus.AppModuleBasic{},
)
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remove it, the Codec should be manually defined by users who wants to use local mode node.

@dadamu dadamu marked this pull request as draft February 7, 2024 10:38
@dadamu dadamu marked this pull request as ready for review February 7, 2024 10:39
@@ -49,7 +48,7 @@ type Database interface {

// SaveMessage stores a single message.
// An error is returned if the operation fails.
SaveMessage(msg *types.Message) error
SaveMessage(height int64, txHash string, msg types.Message, addresses []string) error
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Message is unmarshaled from TxBody which does not contain height, txHash and addresses information, so I change the interface to include them.

@dadamu dadamu requested a review from MonikaCat February 7, 2024 10:55
@dadamu dadamu requested review from leobragaz, 0x7u and icfor March 11, 2024 13:43
Copy link

@0x7u 0x7u left a comment

Choose a reason for hiding this comment

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

LGTM.

@dadamu just for me to understand better could you explain:

  • What's a codec regarding transactions?
  • How do you decide the data structure/layout your mapping to?

@dadamu
Copy link
Contributor Author

dadamu commented Mar 13, 2024

@0x7u Thanks for the review!

What's a codec regarding transactions?

Currently, Cosmos-SDK handles the data in ProtoBuffer format. In addition, Codec is the encoder/decoder for ProtoBuffer, which is responsible to decode the raw data in ProtoBuffer format into the golang structure. This document tells more information.

Currently, in order to transform all the message from ProtoBuffer into JSON to store inside messages table, it is required to register all needed ProtoBuffer messages into Codec. But it is hard to maintain the Codec register since it is not easy to track what custom messages being used by a chain. Say, there are custom messages MsgSaveProfile, MsgCreatePost and etc., in Desmos, while there are MsgClaimStray, MsgAttest and etc. ins Jackal. So this PR tries to get JSON-parsed response from API server instead of ProtoBuffer encoded response from gRPC server so that Juno can get rid of Codec register issue.

How do you decide the data structure/layout your mapping to?

The structure/layout is following the Cosmos-SDK spec. Although the original structure is based on ProtoBuffer, so some Any type data is required to be rewritten into JSON format. The reference is here.

Copy link
Contributor

@MonikaCat MonikaCat left a comment

Choose a reason for hiding this comment

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

utACK

@dadamu dadamu merged commit 913173f into cosmos/v0.47.x Mar 19, 2024
4 checks passed
@dadamu dadamu deleted the paul/SDK-72/eliminate-sdk-codec-in-remote-mode branch March 19, 2024 02:48
dzmitryhil pushed a commit to CoreumFoundation/junov5 that referenced this pull request Dec 16, 2024
<!-- < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < ☺
v                               ✰  Thanks for creating a PR! ✰    
v    Before smashing the submit button please review the checkboxes.
v If a checkbox is n/a - please still include it but + a little note why
☺ > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >  -->

## Description
<!-- Small description -->
This PR eliminate the codec dependency from remote node to make Juno
chain-agnostic.

## Checklist
- [x] Targeted PR against correct branch.
- [ ] Linked to Github issue with discussion and accepted design OR link
to spec that describes this work.
- [ ] Wrote unit tests.  
- [x] Re-reviewed `Files changed` in the Github PR explorer.
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

Successfully merging this pull request may close these issues.

3 participants