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

codec: Rename codec and marshaler interfaces #9226

Merged
merged 8 commits into from
Apr 29, 2021
Merged

Conversation

robert-zaremba
Copy link
Collaborator

@robert-zaremba robert-zaremba commented Apr 28, 2021

Renames. Trying to standarize the interfaces. See the issue for more details.

closes: #8413


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

@codecov
Copy link

codecov bot commented Apr 28, 2021

Codecov Report

Merging #9226 (8bc66b6) into master (c94e9eb) will not change coverage.
The diff coverage is 82.05%.

❗ Current head 8bc66b6 differs from pull request most recent head 061b173. Consider uploading reports for the commit 061b173 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master    #9226   +/-   ##
=======================================
  Coverage   60.03%   60.03%           
=======================================
  Files         595      595           
  Lines       37353    37353           
=======================================
  Hits        22425    22425           
  Misses      12940    12940           
  Partials     1988     1988           
Impacted Files Coverage Δ
client/context.go 54.86% <ø> (ø)
codec/yaml.go 50.00% <ø> (ø)
server/rosetta.go 0.00% <ø> (ø)
simapp/app.go 83.09% <ø> (ø)
simapp/genesis.go 100.00% <ø> (ø)
simapp/state.go 0.00% <ø> (ø)
simapp/utils.go 17.85% <ø> (ø)
testutil/network/network.go 90.00% <ø> (ø)
types/module/configurator.go 18.51% <ø> (ø)
types/module/module.go 68.36% <ø> (ø)
... and 96 more

@github-actions github-actions bot added C:x/authz C:x/bank C:x/capability C:x/crisis C:x/distribution distribution module related C:x/evidence C:x/feegrant C:x/genutil genutil module issues C:x/gov C:x/mint C:x/params C:x/slashing C:x/staking C:x/upgrade T: ADR An issue or PR relating to an architectural decision record T:Docs Changes and features related to documentation. labels Apr 29, 2021
robert-zaremba and others added 2 commits April 29, 2021 12:24
Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com>
@robert-zaremba
Copy link
Collaborator Author

I am not sure if this is relevant. I have found some instances of codec.Marshaler in md files and comments. Should you rename it to codec.Codec?

Thanks for checking, yes not all places in docs were updated. I've revisit it and updated.

Co-authored-by: Marko <marbar3778@yahoo.com>
Copy link
Contributor

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

lgtm, thanks!

@amaury1093 amaury1093 added the A:automerge Automatically merge PR once all prerequisites pass. label Apr 29, 2021
@mergify mergify bot merged commit be4a965 into master Apr 29, 2021
@mergify mergify bot deleted the robert/marshaler-rename branch April 29, 2021 10:46
@colin-axner
Copy link
Contributor

colin-axner commented May 3, 2021

Extra: I removed the BinaryBare part of the Marshal* methods / interface. It's really something Amino specific (no other codec has that name). Since, in SDK we never use Amino directly (only through LegacyAmino wrapper), we can easily update interfaces and remove it.
If folks don't like it, I can rollback that commit.

We happened to be using these functions in IBC. Is there a function I can migrate to that does the exact same functionality? We cannot change encoding values (ie they need to be exactly the same as before)

Is it safe to go from MustUnmarshalBinaryBare -> MustUnmarshal and the same for Marshal? I assume it should be fine since we are using proto?

Edit: I misread the CHANGELOG. I realize it is just the suffix that was removed (not the function itself) 👍

MarshalBinaryBare(o ProtoMarshaler) ([]byte, error)
MustMarshalBinaryBare(o ProtoMarshaler) []byte
BinaryCodec interface {
// Marshal returns binary encoding of v.
Copy link
Contributor

Choose a reason for hiding this comment

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

What is v in the comment?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah should be o. @xavierlepretre would you like to create a quick PR?

larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023
* codec: Rename codec and marshaler interfaces, ref: 8413

* codec: remove BinaryBare

* changelog update

* Update comments and documentation

* adding doc string comments

* Update CHANGELOG.md

Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com>

* Update codec/codec.go

Co-authored-by: Marko <marbar3778@yahoo.com>

Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com>
Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com>
Co-authored-by: Marko <marbar3778@yahoo.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass. C:CLI C:Cosmovisor Issues and PR related to Cosmovisor C:Simulations C:x/auth C:x/authz C:x/bank C:x/capability C:x/crisis C:x/distribution distribution module related C:x/evidence C:x/feegrant C:x/genutil genutil module issues C:x/gov C:x/mint C:x/params C:x/slashing C:x/staking C:x/upgrade T: ADR An issue or PR relating to an architectural decision record T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine). T:Docs Changes and features related to documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename codec.Marshaler
7 participants