ADR 109: Internalize specific packages to reduce surface area by thanethomson · Pull Request #1485 · cometbft/cometbft

thanethomson · 2023-10-11T21:29:06Z

Partially addresses #1484, based on #1483.

One concern I have is around the possibility that this PR internalizes certain types that are present in publicly accessible function signatures as parameters or return types. This will take some work to address.

I've already confirmed that the linter being run on the Cosmos SDK main branch doesn't pick up any missing code-related issues at this point.

This PR currently internalizes about 64k lines of code.

PR checklist

Tests written/updated
Changelog entry added in .changelog (we use unclog to manage our changelog)
Updated relevant documentation (docs/ or spec/) and code comments

Signed-off-by: Thane Thomson <connect@thanethomson.com>

lasarojc

The changes, which are many but all straightforward, look good to me.
Regarding the use of internal structures as parameters and results in public API, isn't that detected by the linter?

lasarojc · 2023-10-13T14:25:29Z

abci/client/local_client.go


 func (app *localClient) LoadSnapshotChunk(ctx context.Context,
-	req *types.RequestLoadSnapshotChunk) (*types.ResponseLoadSnapshotChunk, error) {
+	req *types.RequestLoadSnapshotChunk,


If we are formatting this way, then there are other places that should be changed.
I would rather make all such changes together.

Please see #604.

thanethomson · 2023-10-16T11:43:34Z

Regarding the use of internal structures as parameters and results in public API, isn't that detected by the linter?

It seems not. See https://github.com/cometbft/go-internalized-example

* ADR 109: Internalize specific packages to reduce surface area (#1485) * consensus: Move into internal Signed-off-by: Thane Thomson <connect@thanethomson.com> * evidence: Move into internal Signed-off-by: Thane Thomson <connect@thanethomson.com> * inspect: Move into internal Signed-off-by: Thane Thomson <connect@thanethomson.com> * state: Move into internal Signed-off-by: Thane Thomson <connect@thanethomson.com> * blocksync: Move into internal Signed-off-by: Thane Thomson <connect@thanethomson.com> * statesync: Move into internal Signed-off-by: Thane Thomson <connect@thanethomson.com> * store: Move into internal Signed-off-by: Thane Thomson <connect@thanethomson.com> * libs/async: Move into internal Signed-off-by: Thane Thomson <connect@thanethomson.com> * libs/autofile: Move into internal Signed-off-by: Thane Thomson <connect@thanethomson.com> * libs/bits: Move into internal Signed-off-by: Thane Thomson <connect@thanethomson.com> * libs/clist: Move into internal Signed-off-by: Thane Thomson <connect@thanethomson.com> * libs/cmap: Move into internal Signed-off-by: Thane Thomson <connect@thanethomson.com> * libs/events: Move into internal Signed-off-by: Thane Thomson <connect@thanethomson.com> * libs/fail: Move into internal Signed-off-by: Thane Thomson <connect@thanethomson.com> * libs/flowrate: Move into internal Signed-off-by: Thane Thomson <connect@thanethomson.com> * libs/os: Move into internal Signed-off-by: Thane Thomson <connect@thanethomson.com> * libs/progressbar: Move into internal Signed-off-by: Thane Thomson <connect@thanethomson.com> * libs/protoio: Move into internal Signed-off-by: Thane Thomson <connect@thanethomson.com> * libs/pubsub: Move into internal Signed-off-by: Thane Thomson <connect@thanethomson.com> * libs/net: Move into internal Signed-off-by: Thane Thomson <connect@thanethomson.com> * libs/rand: Move into internal Signed-off-by: Thane Thomson <connect@thanethomson.com> * libs/service: Move into internal Signed-off-by: Thane Thomson <connect@thanethomson.com> * libs/strings: Move into internal Signed-off-by: Thane Thomson <connect@thanethomson.com> * libs/sync: Move into internal Signed-off-by: Thane Thomson <connect@thanethomson.com> * libs/tempfile: Move into internal Signed-off-by: Thane Thomson <connect@thanethomson.com> * libs/timer: Move into internal Signed-off-by: Thane Thomson <connect@thanethomson.com> * Add changelog entries Signed-off-by: Thane Thomson <connect@thanethomson.com> --------- Signed-off-by: Thane Thomson <connect@thanethomson.com> * ADR 109: Modularize test infra (#1488) * test/e2e: Split out as separate module Signed-off-by: Thane Thomson <connect@thanethomson.com> * test/loadtime: Split out as separate module Signed-off-by: Thane Thomson <connect@thanethomson.com> * test/e2e: Remove optimization from Docker image construction Signed-off-by: Thane Thomson <connect@thanethomson.com> * Ensure that linter covers E2E framework and app Signed-off-by: Thane Thomson <connect@thanethomson.com> * Update CI linting to cover submodules Signed-off-by: Thane Thomson <connect@thanethomson.com> * Add changelog entry Signed-off-by: Thane Thomson <connect@thanethomson.com> * Expand linter coverage to loadtime tool Signed-off-by: Thane Thomson <connect@thanethomson.com> * Add missing phony entries Signed-off-by: Thane Thomson <connect@thanethomson.com> * test/e2e: Sync debug Dockerfile with primary Dockerfile Signed-off-by: Thane Thomson <connect@thanethomson.com> --------- Signed-off-by: Thane Thomson <connect@thanethomson.com> * chore: ADR 109: go mod tidy (#1606) * go mod tidy Signed-off-by: Thane Thomson <connect@thanethomson.com> * go.mod: Remove patch version Signed-off-by: Thane Thomson <connect@thanethomson.com> * go.mod: Remove new toolchain directives Signed-off-by: Thane Thomson <connect@thanethomson.com> --------- Signed-off-by: Thane Thomson <connect@thanethomson.com> * ADR 109: Fix mock generation (#1607) * internal: Fix mockery code generation script paths Signed-off-by: Thane Thomson <connect@thanethomson.com> * make mockery Signed-off-by: Thane Thomson <connect@thanethomson.com> --------- Signed-off-by: Thane Thomson <connect@thanethomson.com> --------- Signed-off-by: Thane Thomson <connect@thanethomson.com>

thanethomson added 26 commits October 11, 2023 20:09

consensus: Move into internal

c8e7319

Signed-off-by: Thane Thomson <connect@thanethomson.com>

evidence: Move into internal

4f5a9cc

Signed-off-by: Thane Thomson <connect@thanethomson.com>

inspect: Move into internal

19c8198

Signed-off-by: Thane Thomson <connect@thanethomson.com>

state: Move into internal

b31ca9d

Signed-off-by: Thane Thomson <connect@thanethomson.com>

blocksync: Move into internal

5408521

Signed-off-by: Thane Thomson <connect@thanethomson.com>

statesync: Move into internal

1b774fd

Signed-off-by: Thane Thomson <connect@thanethomson.com>

store: Move into internal

4288b5f

Signed-off-by: Thane Thomson <connect@thanethomson.com>

libs/async: Move into internal

75870a2

Signed-off-by: Thane Thomson <connect@thanethomson.com>

libs/autofile: Move into internal

40c10fc

Signed-off-by: Thane Thomson <connect@thanethomson.com>

libs/bits: Move into internal

c10cb21

Signed-off-by: Thane Thomson <connect@thanethomson.com>

libs/clist: Move into internal

8094171

Signed-off-by: Thane Thomson <connect@thanethomson.com>

libs/cmap: Move into internal

5add0da

Signed-off-by: Thane Thomson <connect@thanethomson.com>

libs/events: Move into internal

a51ff32

Signed-off-by: Thane Thomson <connect@thanethomson.com>

libs/fail: Move into internal

69360d3

Signed-off-by: Thane Thomson <connect@thanethomson.com>

libs/flowrate: Move into internal

851e779

Signed-off-by: Thane Thomson <connect@thanethomson.com>

libs/os: Move into internal

48958f0

Signed-off-by: Thane Thomson <connect@thanethomson.com>

libs/progressbar: Move into internal

b745c1f

Signed-off-by: Thane Thomson <connect@thanethomson.com>

libs/protoio: Move into internal

95f5886

Signed-off-by: Thane Thomson <connect@thanethomson.com>

libs/pubsub: Move into internal

7c519d7

Signed-off-by: Thane Thomson <connect@thanethomson.com>

libs/net: Move into internal

07bfe44

Signed-off-by: Thane Thomson <connect@thanethomson.com>

libs/rand: Move into internal

e810e17

Signed-off-by: Thane Thomson <connect@thanethomson.com>

libs/service: Move into internal

ac1c019

Signed-off-by: Thane Thomson <connect@thanethomson.com>

libs/strings: Move into internal

52ba5ef

Signed-off-by: Thane Thomson <connect@thanethomson.com>

libs/sync: Move into internal

7eabb94

Signed-off-by: Thane Thomson <connect@thanethomson.com>

libs/tempfile: Move into internal

57c273d

Signed-off-by: Thane Thomson <connect@thanethomson.com>

libs/timer: Move into internal

3d7b4c7

Signed-off-by: Thane Thomson <connect@thanethomson.com>

thanethomson added this to the 2023-Q4 milestone Oct 11, 2023

thanethomson mentioned this pull request Oct 11, 2023

ADR 109: Reduce Go API surface area #1484

Closed

3 tasks

thanethomson changed the title ~~ADR 109: Internalize non-critical code~~ ADR 109: Internalize packages Oct 11, 2023

thanethomson changed the title ~~ADR 109: Internalize packages~~ ADR 109: Internalize specific packages to reduce surface area Oct 11, 2023

Add changelog entries

cb19064

Signed-off-by: Thane Thomson <connect@thanethomson.com>

thanethomson marked this pull request as ready for review October 12, 2023 11:36

thanethomson requested a review from a team as a code owner October 12, 2023 11:36

thanethomson requested a review from a team October 12, 2023 11:36

lasarojc approved these changes Oct 13, 2023

View reviewed changes

thanethomson merged commit 1511af9 into feature/adr109-reduce-go-api-surface Oct 16, 2023

thanethomson deleted the thane/adr109/internalize branch October 16, 2023 11:44

thanethomson restored the thane/adr109/internalize branch October 16, 2023 12:02

thanethomson deleted the thane/adr109/internalize branch October 16, 2023 12:04

thanethomson mentioned this pull request Nov 13, 2023

ADR 109: Fix mock generation #1607

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ADR 109: Internalize specific packages to reduce surface area#1485

ADR 109: Internalize specific packages to reduce surface area#1485
thanethomson merged 27 commits intofeature/adr109-reduce-go-api-surfacefrom
thane/adr109/internalize

thanethomson commented Oct 11, 2023 •

edited

Loading

Uh oh!

lasarojc left a comment

Uh oh!

lasarojc Oct 13, 2023

Uh oh!

thanethomson Oct 16, 2023

Uh oh!

thanethomson commented Oct 16, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

thanethomson commented Oct 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR checklist

Uh oh!

lasarojc left a comment

Choose a reason for hiding this comment

Uh oh!

lasarojc Oct 13, 2023

Choose a reason for hiding this comment

Uh oh!

thanethomson Oct 16, 2023

Choose a reason for hiding this comment

Uh oh!

thanethomson commented Oct 16, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

thanethomson commented Oct 11, 2023 •

edited

Loading