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.

Sign up for GitHub

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

Jump to bottom

fix: contract bundles have duplicate dependencies #34

Merged
merged 4 commits into from
Jan 4, 2024
Merged

fix: contract bundles have duplicate dependencies #34

merged 4 commits into from
Jan 4, 2024

Conversation

0xpatrickdev
Copy link
Member

  • forces npm to resolve one version for each agoric-sdk dependency 3c1cc85
  • adds yarn build to ci
Scenario Bundle ID File Size
Before b1-1e1aeca....ad8e572 1.8M
b1-34a17f3....40810d 1.6M
After b1-60228f8....21ae8 747K
b1-2767ddb....07b8b 584K

I also tested with the changes in #28:

diff --git a/contract/src/gameAssetContract.js b/contract/src/gameAssetContract.js
index 90c0cfc..63680e9 100644
--- a/contract/src/gameAssetContract.js
+++ b/contract/src/gameAssetContract.js
@@ -5,6 +5,7 @@ import { Far } from '@endo/far';
 import { M, getCopyBagEntries } from '@endo/patterns';
 import { AmountMath, AssetKind } from '@agoric/ertp/src/amountMath.js';
 import { AmountShape } from '@agoric/ertp/src/typeGuards.js';
+import { assertIssuerKeywords } from "@agoric/zoe/src/contractSupport/index.js";
 // Use the deprecated atomicRearrange API
 // for compatibility with mainnet1B.
 import '@agoric/zoe/exported.js';
@@ -27,6 +28,7 @@ const bagValueSize = amt => {
  * @param {ZCF<{joinPrice: Amount}>} zcf
  */
 export const start = async zcf => {
+  assertIssuerKeywords(zcf, harden(['Price']));
   const { joinPrice } = zcf.getTerms();

   const { zcfSeat: gameSeat } = zcf.makeEmptySeatKit();

And am observing smaller bundles:

Scenario Bundle ID File Size
Before b1-e14ac9e....f0052 4.9M
b1-96191af....04cd8a 1.6M
After b1-90efd17....3ca9 1.5M
b1-2767ddb....07b8b 584K

@0xpatrickdev
fix(contract): multiple versions of dependencies in generated bundles
3c1cc85 
- use 'resolutions' to ensure only one version of each package is installed
- remove babel resolutions (fe only) from contract package.json
- confirmed changes via 'find ./node_modules -type d -name 'endo''
@0xpatrickdev
feat(contract): add build script
5e23ec1 
- currently, the contract is built in the docker container. this change allows the consumer to build the contract outside the docker container, or in ci
@0xpatrickdev
chore(ci): include 'yarn build' step
cf3ac26
@0xpatrickdev 0xpatrickdev requested a review from dckc January 3, 2024 22:21
dckc
dckc approved these changes Jan 3, 2024
View reviewed changes
Copy link
Member

@dckc dckc left a comment

Choose a reason for hiding this comment

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

I suggest documenting that the resolutions are a work-around.

.github/workflows/pr.yml
@@ -18,5 +18,7 @@ jobs:
run: yarn
- name: yarn lint
run: yarn lint
- name: yarn build
run: yarn build
Copy link
Member

Choose a reason for hiding this comment

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

We should test that the bundle, when compressed, is <1MB.
Want to do it in this PR or leave it for later?

Copy link
Member Author

Choose a reason for hiding this comment

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

Our heads are in the same place!

I think a separate PR is best. I also wonder if agoric run / writeCoreProposal should be responsible for enforcing this (or, at least warning)

Copy link
Member Author

Choose a reason for hiding this comment

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

New issue here: #36. Can move it to agoric-sdk if you agree this should be a responsibility of agoric run

contract/package.json
@@ -11,7 +11,7 @@
"docker:make": "docker compose exec agd make -C /workspace/contract",
"make:help": "make list",
"start": "yarn docker:make clean start-contract print-key",
"build": "exit 0",
"build": "agoric run scripts/build-game1-start.js",
Copy link
Member

Choose a reason for hiding this comment

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

I'm a little nervous about permissions conflicting with doing this build in docker, but I guess it's OK for now.

package.json Show resolved Hide resolved
yarn.lock
@@ -1381,31 +1338,6 @@
"@endo/zip" "^0.2.31"
ses "^0.18.4"

"@endo/compartment-mapper@^0.8.4", "@endo/compartment-mapper@^0.8.5":
Copy link
Member

Choose a reason for hiding this comment

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

👏

many fewer dependencies

@0xpatrickdev @dckc
docs(contract): add note about npm resolutions
b5b1439 
Co-authored-by: Dan Connolly <connolly@agoric.com>
@0xpatrickdev 0xpatrickdev merged commit 2754328 into main Jan 4, 2024
1 check passed
@0xpatrickdev 0xpatrickdev deleted the npm-resolutions branch January 4, 2024 00:07
This was referenced Jan 4, 2024
Closed
Closed
@0xpatrickdev 0xpatrickdev mentioned this pull request Jan 24, 2024
Merged
@dckc dckc mentioned this pull request Aug 6, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dckc dckc dckc approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@0xpatrickdev @dckc