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

orchestration bundles: remove unused protobuf encoders/decoders in CosmosOrchestrationAccount #9578

Closed
0xpatrickdev opened this issue Jun 25, 2024 · 2 comments · Fixed by #9743
Assignees
Labels
enhancement New feature or request

Comments

@0xpatrickdev
Copy link
Member

0xpatrickdev commented Jun 25, 2024

What is the Problem Being Solved?

An analysis of a generated bundle for CosmosOrchestrationAccount reveals that deep imports of protobuf message encoders/decoders from @agoric/cosmic-proto do not result in any tree shaking or dead code elimination.

This issue was uncovered with guidance from @kriskowal:

bundle analysis
$ du -sh b1-d77d97b918b5627de26bcdac23e11d69b446df4907fd5de7e902036fa8bbc41428599dd0729e1ad68f57311ea29cc663f9fa8b243c020794d03a9a51ed5fdbd0.json
4.4M

$ < b1-d77d97b918b5627de26bcdac23e11d69b446df4907fd5de7e902036fa8bbc41428599dd0729e1ad68f57311ea29cc663f9fa8b243c020794d03a9a51ed5fdbd0.json jq -r .endoZipBase64 | base64 -d > bundle.zip

$ unzip bundle.zip

$ unzip bundle.zip | wc -l
     342

$ du -sh *
3.1M	@agoric
 20K	@cosmjs
604K	@endo
 72K	bn.js-v5.2.0
180K	compartment-map.json
 28K	jessie.js-v0.3.4

$ cd @agoric && du -sh *
4.0K	assert-v0.6.0
 44K	base-zone-v0.1.0
2.4M	cosmic-proto-v0.4.0
100K	ertp-v0.16.2
 72K	internal-v0.3.2
 80K	notifier-v0.6.2
 40K	orchestration-v0.1.0
 48K	store-v0.9.2
 24K	time-v0.3.2
 28K	vat-data-v0.5.2
 76K	vow-v0.1.0
136K	zoe-v0.26.2
 12K	zone-v0.2.2
 
$ find cosmic-proto-v0.4.0 | wc -l
     260

$ ls cosmic-proto-v0.4.0/dist/codegen/
agoric		cosmos		google		icq		json-safe.js	utf8.js
amino		cosmos_proto	helpers.js	ics23		proofs.js	varint.js
binary.js	gogoproto	ibc		index.js	tendermint

$ ls @agoric/cosmic-proto-v0.4.0/dist/codegen/cosmos
auth		bank		bundle.js	distribution	gov		mint		staking		upgrade
authz		base		crypto		feegrant	group		params		tx		vesting

Notably, at least authz, feegrant, gov, mint, and upgrade modules are not used. For modules like staking and distribution, we wouldn't expect to see files like genesis.js in the export. We also wouldn't expect to see unused messages such as MsgEditValidator in staking/v1beta1/tx.js.

Note: The current approach of protobuf serialization in JS (as opposed to Go + a bridge) is necessitated by @agoric/orchestration's requirement to support arbitrary messages. While this design choice is outside the scope of this issue, a separate discussion on this topic is welcome.

Description of the Design

This is primarily a Spike to explore potential solutions. Areas to investigate include:

Scaling Considerations

The primary goal of this ticket is to prevent publishing unused code to the chain.

A secondary consideration is the development workflow for external developers. If they need to support a new chain or use case with their own protos, they shouldn't have to upstream changes to @agoric/cosmic-proto to get swingset-compatible encoder/decoder support. This might suggest a more localized approach, where codegen happens on a ~per-contract basis in @agoric/orchestration.

Test Plan

TBD

Upgrade Considerations

N/A

@0xpatrickdev 0xpatrickdev added the enhancement New feature or request label Jun 25, 2024
@pyramation
Copy link

So initially we're rebuilding cosmjs to remove the dependency of cosmjs-types

then the plan is to look at, what structure does telescope need to print in order to work properly? Protobuf-es won't support json encoding, or other features, so I would suggest while it's good if you're doing generic proto stuff, it's not a solution for cosmos-sdk protos

I'll ping you guys in slack so we can iron this out and get whatever is needed on the roadmap

@kriskowal
Copy link
Member

kriskowal commented Jun 25, 2024

Just as a point of terminology, “tree shaking” is a compiler feature that identifies unreachable/dead code and avoids regenerating it. This is a feature of bundlers like Rollup which can be used under different circumstances than ours.

I do not know of any term that is as pithy as “tree shaking” for “designing modules such that they do not incidentally import functionality that the importer may not need”.

However, a “barrel module” is “a module that reexports many other modules” that will incidentally tend to import lots of code the importer is unlikely to need.

So, I would coin the term “unbarreled packages” for packages that export finely scoped modules that do not incidentally import anything the importer is unlikely to need.

@turadg turadg self-assigned this Jul 19, 2024
@mergify mergify bot closed this as completed in #9743 Jul 22, 2024
@mergify mergify bot closed this as completed in ee2126b Jul 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants