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

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 <[email protected]>

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

@@ -135,15 +135,17 @@ func (app *localClient) OfferSnapshot(ctx context.Context, req *types.RequestOff
 }

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

thanethomson added 26 commits October 11, 2023 20:09

consensus: Move into internal

c8e7319

Signed-off-by: Thane Thomson <[email protected]>

evidence: Move into internal

4f5a9cc

Signed-off-by: Thane Thomson <[email protected]>

inspect: Move into internal

19c8198

Signed-off-by: Thane Thomson <[email protected]>

state: Move into internal

b31ca9d

Signed-off-by: Thane Thomson <[email protected]>

blocksync: Move into internal

5408521

Signed-off-by: Thane Thomson <[email protected]>

statesync: Move into internal

1b774fd

Signed-off-by: Thane Thomson <[email protected]>

store: Move into internal

4288b5f

Signed-off-by: Thane Thomson <[email protected]>

libs/async: Move into internal

75870a2

Signed-off-by: Thane Thomson <[email protected]>

libs/autofile: Move into internal

40c10fc

Signed-off-by: Thane Thomson <[email protected]>

libs/bits: Move into internal

c10cb21

Signed-off-by: Thane Thomson <[email protected]>

libs/clist: Move into internal

8094171

Signed-off-by: Thane Thomson <[email protected]>

libs/cmap: Move into internal

5add0da

Signed-off-by: Thane Thomson <[email protected]>

libs/events: Move into internal

a51ff32

Signed-off-by: Thane Thomson <[email protected]>

libs/fail: Move into internal

69360d3

Signed-off-by: Thane Thomson <[email protected]>

libs/flowrate: Move into internal

851e779

Signed-off-by: Thane Thomson <[email protected]>

libs/os: Move into internal

48958f0

Signed-off-by: Thane Thomson <[email protected]>

libs/progressbar: Move into internal

b745c1f

Signed-off-by: Thane Thomson <[email protected]>

libs/protoio: Move into internal

95f5886

Signed-off-by: Thane Thomson <[email protected]>

libs/pubsub: Move into internal

7c519d7

Signed-off-by: Thane Thomson <[email protected]>

libs/net: Move into internal

07bfe44

Signed-off-by: Thane Thomson <[email protected]>

libs/rand: Move into internal

e810e17

Signed-off-by: Thane Thomson <[email protected]>

libs/service: Move into internal

ac1c019

Signed-off-by: Thane Thomson <[email protected]>

libs/strings: Move into internal

52ba5ef

Signed-off-by: Thane Thomson <[email protected]>

libs/sync: Move into internal

7eabb94

Signed-off-by: Thane Thomson <[email protected]>

libs/tempfile: Move into internal

57c273d

Signed-off-by: Thane Thomson <[email protected]>

libs/timer: Move into internal

3d7b4c7

Signed-off-by: Thane Thomson <[email protected]>

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 <[email protected]>

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

thanethomson requested review from a team as code owners 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
16 checks passed

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 commented Oct 11, 2023 •

edited

Loading

lasarojc left a comment

lasarojc Oct 13, 2023

thanethomson Oct 16, 2023

thanethomson commented Oct 16, 2023

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

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

Conversation

thanethomson commented Oct 11, 2023 • edited Loading

PR checklist

lasarojc left a comment

Choose a reason for hiding this comment

lasarojc Oct 13, 2023

Choose a reason for hiding this comment

thanethomson Oct 16, 2023

Choose a reason for hiding this comment

thanethomson commented Oct 16, 2023

thanethomson commented Oct 11, 2023 •

edited

Loading