style!: Refactor codebase to use proper Interface prefixes #13268

Olshansk · 2022-09-13T23:10:51Z

Description

Per the discussion and examples in #13254, this aims to make the Cosmos SDK more compliant with Golang syntax best practices.

Changes

Specifically, naming interface with either an I prefix or an Interface suffix rather than an I suffix. The interfaces renamed are:

s/ValidatorI/IValidator
s/HasAnimalI/IHasAnimal
s/MsgSubmitEvidenceI/IMsgSubmitEvidence
s/DelegationI/IDelegation
s/FeeAllowanceI/IFeeAllowance
s/ModuleAccountI/IModuleAccount
s/AccountKeeperI/IAccountKeeperI
s/AccountI/IAccount

How Changes were made

All relevant interface were found using VSCode's regex and case match for: type (.*)[a-z]I Interface \{
The following two regex find and replace all was made for each interface (Account is an example):

Find: AccountI([^a-z^A-z])
Replace: IAccount$1

Find: AccountI$
Replace: Account

References

Interface naming convention in Golang:

Dave Cheney's Practical Go:

Author Checklist

Reviewers Checklist

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

Olshansk · 2022-09-13T23:16:31Z

@alexanderbez The only thing I did to verify is running make test. Should that be enough given the details here?

docs/core/encoding.md

julienrbrt · 2022-09-14T08:24:21Z

CHANGELOG.md

@@ -37,6 +37,8 @@ Ref: https://keepachangelog.com/en/1.0.0/

 ## [Unreleased]

+* (improvement) [#13254](https://github.com/cosmos/cosmos-sdk/pull/13268) Change `I` suffix to prefix in the relevant Golang interfaces


Can you move that under the API Breaking Changes section (l.107)?

Done. Note that I was following the instructions in https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#pull-requests and figured someone (in the future) is responsible from moving it from Unreleased to API Breaking.

julienrbrt · 2022-09-14T08:33:30Z

x/auth/README.md

-  * [CLI](#cli)
-  * [gRPC](#grpc)
-  * [REST](#rest)
+- [`x/auth`](#xauth)


Let's apply the correct .md formatting here too. (When you use the markdown all in one vscode extension, it unfortunately generates a ToC that does not respect the markdownlint config. You need to ctrl+z and ctrl+k+ctrl+s for reverting the formatting)

Yea, Visual Studio's "Markdown all in one" doesn't play well with "MarkdownLint". I ended up just checking out all of my .md changes and updating it manually.

codecov · 2022-09-14T08:47:01Z

Codecov Report

Merging #13268 (99ba23a) into main (4fe7797) will decrease coverage by 0.22%.
The diff coverage is 6.66%.

❗ Current head 99ba23a differs from pull request most recent head 83009d6. Consider uploading reports for the commit 83009d6 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #13268      +/-   ##
==========================================
- Coverage   55.87%   55.65%   -0.23%     
==========================================
  Files         646      712      +66     
  Lines       54895    60840    +5945     
==========================================
+ Hits        30675    33862    +3187     
- Misses      21762    24162    +2400     
- Partials     2458     2816     +358

Impacted Files	Coverage Δ
baseapp/abci.go	`67.01% <0.00%> (+2.59%)`	⬆️
baseapp/baseapp.go	`77.12% <ø> (+0.25%)`	⬆️
baseapp/grpcrouter.go	`90.00% <ø> (ø)`
baseapp/grpcrouter_helpers.go	`25.00% <ø> (ø)`
baseapp/grpcserver.go	`1.72% <ø> (ø)`
baseapp/msg_service_router.go	`85.29% <ø> (+4.41%)`	⬆️
baseapp/options.go	`69.23% <ø> (+0.71%)`	⬆️
client/account_retriever.go	`0.00% <ø> (ø)`
client/cmd.go	`57.73% <0.00%> (ø)`
client/context.go	`54.49% <0.00%> (-1.79%)`	⬇️
... and 260 more

amaury1093 · 2022-09-14T12:14:53Z

x/authz/expected_keepers.go

-	GetAccount(ctx sdk.Context, addr sdk.AccAddress) authtypes.AccountI
-	NewAccountWithAddress(ctx sdk.Context, addr sdk.AccAddress) authtypes.AccountI
-	SetAccount(ctx sdk.Context, acc authtypes.AccountI)
+	GetAccount(ctx sdk.Context, addr sdk.AccAddress) authtypes.IAccount


This is a huge API breaking change IMO, for all chains. With time I got used to AccountI. I also don't see a strong need to switch (just because some guy wrote about it in a medium article?)

I'm fine with eventually switching to something consistent, but maybe let's do it in 2 phases:

Add type aliases, e.g. AccountI -> IAccount, so that everything is backwards-compatible. Add deprecation godoc on all old interfaces.

Rip off the old interfaces, but probably not in 0.47

This is a huge API breaking change IMO, for all chains

Agreed.

I also don't see a strong need to switch

Yea, I was conflicted as well so I made sure to double-check if I should proceed before making any changes: #13254 (comment)

(just because some guy wrote about it in a medium article?)

It seems to be the "Go" standard. I can find more references and added them to the PR. Just lmk if I should invest the time

Add type aliases, e.g. AccountI -> IAccount, so that everything is backwards-compatible. Add deprecation godoc on all old interfaces.

Done. See #13279

Here's an example:

Rip off the old interfaces, but probably not in 0.47

Could you leave a comment in #13279 on next steps and timelines?

I appreciate your efforts here @Olshansk and I know we're not really treating the SDK as 1.0 yet so some breaking changes are okay.

At the same time, I think we really need to measure the value of breaking changes like this. I generally think changing things just because some authority says the style should be different isn't a great reason to force downstream changes on clients...

So my preference would generally for us not to be spending time on these sort of changes (and then forcing our users to spend time on them), if there isn't some larger benefit.

AccountI, etc. may seem clunky but it also seems like not a big deal...

Understood.

I confirmed the scope of work before starting in the original tickets, tended to the comments in the PR, documented the future work, tested the changes, and made it backwards compatible with the goal of better code health before the SDK reaches 1.0

If you still believe I should close out the PR and issue without a merge, please lmk.

The aliases address backwards compatibility. Also if its an issue for downstream users, we don't have to remove them for 1.0

@AmauryM @marbar3778 what do you think?

So you mean you defined new proto types? Or just aliases? Idk...just seems like such a large diff and tech debt to maintain for a version just to change type names.

I implemented the approach @AmauryM suggested in the first message in this thread.

Given the long async back and forth, is there a cosmos contributor hour where we can discuss it over a call?

@AmauryM Could you take a look at the latest changes and see if it aligns with your suggestion?

My personal take is that this PR sees a lot of resistance, and doesn't have a clear motivation from SDK users (i.e. no-one came to us about AccountI vs IAccount).

@Olshansk I really appreciate your work here, but my recommendation would be to simply to close the PR. I'm sorry about this, we should have better discussed this in the issue beforehand.

However, if you feel strongly about this going through, please join the SDK community call this Thursday 6-7pm CEST, and we can quickly discuss this with a wider audience before moving forward.

tac0turtle · 2022-10-27T22:27:09Z

closing this for now but let me know if you'd like to attend the community call to discuss further.

Olshansk · 2022-10-28T19:50:52Z

closing this for now but let me know if you'd like to attend the community call to discuss further.

I would! When is the next one?

peterbourgon · 2022-10-30T04:07:12Z

Idiomatic Go interfaces are named by their method set, and are never prefixed or suffixed by I or Interface.

By convention, one-method interfaces are named by the method name plus an -er suffix or similar modification to construct an agent noun: Reader, Writer, Formatter, CloseNotifier etc.

There are a number of such names and it's productive to honor them and the function names they capture. Read, Write, Close, Flush, String and so on have canonical signatures and meanings. To avoid confusion, don't give your method one of those names unless it has the same signature and meaning. Conversely, if your type implements a method with the same meaning as a method on a well-known type, give it the same name and signature; call your string-converter method String not ToString.

https://go.dev/doc/effective_go#interface-names

Olshansk · 2022-10-30T04:13:18Z

@peterbourgon Really appreciate the link & reference (and somewhat embarrassed I didn't find that link myself). Let's close this out.

Olshansk added 15 commits September 13, 2022 18:05

Rename HashAnimal interface

ab9beda

Rename Test Interface

6eee507

Fix HasAnimal interface

d6574b1

Fix AccountI interface

7b15267

Fix AccountKeeperI interface

4e97df8

s/ModuleIAccount/IModuleAccount

c5db262

s/FeeAllowanceI/IFeeAllowance

74fa31a

s/DelegationI/IDelegation

afca2bc

s/ValidatorI/IValidator

a31f551

Fix the MsgSubmitEvidence interface

030e963

Fix supply interface

3972adf

Update other HasAnimal interfaces

0a2c157

Minor ValidatorI fix

8881904

Reverted two markdown files

8a4e4d8

Ran make format

a1afc83

github-actions bot added C:CLI C:Simulations C:x/auth C:x/authz C:x/bank C:x/distribution distribution module related C:x/evidence C:x/feegrant C:x/genutil genutil module issues C:x/gov C:x/group C:x/mint C:x/nft C:x/slashing C:x/staking labels Sep 13, 2022

Update changelog

9352163

github-actions bot added C:Cosmovisor Issues and PR related to Cosmovisor C:orm C:x/upgrade labels Sep 13, 2022

Olshansk mentioned this pull request Sep 13, 2022

[Code Style] Appropriate suffix/prefix for interface definitions #13254

Closed

julienrbrt reviewed Sep 13, 2022

View reviewed changes

docs/core/encoding.md Outdated Show resolved Hide resolved

Olshansk added 4 commits September 13, 2022 20:54

Fix markdown lint violations

0fb6a53

More markdown lint violations fixes

0168434

Manual markdown fixes

4476385

Fix markdown whitespace

99ba23a

julienrbrt reviewed Sep 14, 2022

View reviewed changes

amaury1093 reviewed Sep 14, 2022

View reviewed changes

Olshansk added 3 commits September 14, 2022 10:53

Revert all Markdown changes

a82f4dc

Manual updates/fixes of READMEs

fd2f1ec

Update x/feegrant/README.md

0caf645

Olshansk mentioned this pull request Sep 14, 2022

[Code Style] Follow up on appropriate suffix/prefix for interface definitions (#13254) #13279

Closed

Add interim type aliases

83009d6

tac0turtle closed this Oct 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

style!: Refactor codebase to use proper Interface prefixes #13268

style!: Refactor codebase to use proper Interface prefixes #13268

Olshansk commented Sep 13, 2022 •

edited

Loading

Olshansk commented Sep 13, 2022 •

edited

Loading

julienrbrt Sep 14, 2022

Olshansk Sep 14, 2022

julienrbrt Sep 14, 2022

Olshansk Sep 14, 2022

codecov bot commented Sep 14, 2022 •

edited

Loading

amaury1093 Sep 14, 2022

Olshansk Sep 14, 2022

aaronc Sep 14, 2022 •

edited

Loading

Olshansk Sep 14, 2022

aaronc Sep 14, 2022

alexanderbez Sep 21, 2022

Olshansk Sep 21, 2022

Olshansk Sep 30, 2022

amaury1093 Oct 3, 2022

Olshansk Oct 4, 2022

tac0turtle commented Oct 27, 2022

Olshansk commented Oct 28, 2022

peterbourgon commented Oct 30, 2022 •

edited

Loading

Olshansk commented Oct 30, 2022

		@@ -37,6 +37,8 @@ Ref: https://keepachangelog.com/en/1.0.0/

		## [Unreleased]

		* (improvement) [#13254](https://github.com/cosmos/cosmos-sdk/pull/13268) Change `I` suffix to prefix in the relevant Golang interfaces

style!: Refactor codebase to use proper Interface prefixes #13268

style!: Refactor codebase to use proper Interface prefixes #13268

Conversation

Olshansk commented Sep 13, 2022 • edited Loading

Description

Changes

How Changes were made

References

Author Checklist

Reviewers Checklist

Olshansk commented Sep 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 14, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronc Sep 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tac0turtle commented Oct 27, 2022

Olshansk commented Oct 28, 2022

peterbourgon commented Oct 30, 2022 • edited Loading

Olshansk commented Oct 30, 2022

Olshansk commented Sep 13, 2022 •

edited

Loading

Olshansk commented Sep 13, 2022 •

edited

Loading

codecov bot commented Sep 14, 2022 •

edited

Loading

aaronc Sep 14, 2022 •

edited

Loading

peterbourgon commented Oct 30, 2022 •

edited

Loading