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

feat(contract): linting and typedefs #26

Closed
wants to merge 13 commits into from
Closed
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -22,3 +22,5 @@ dist-ssr
*.njsproj
*.sln
*.sw?

_agstate/yarn-links
74 changes: 54 additions & 20 deletions contract/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -11,23 +11,32 @@
"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": "node_modules/agoric/bin/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.

is node_modules/agoric/bin/agoric necessary? Doesn't yarn arrange for agoric to work?

Copy link
Member

Choose a reason for hiding this comment

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

The scope of this PR seems to be expanding :)

Do you plan to keep the commits separate or squash?

"test": "ava --verbose",
"lint": "eslint '**/*.{js,jsx}'",
"lint-fix": "eslint --fix '**/*.{js,jsx}'",
"lint-fix-jessie": "eslint -c '.eslintrc-jessie.js' --fix '**/*.{js,jsx}'",
"lint-jessie": "eslint -c '.eslintrc-jessie.js' '**/*.{js,jsx}'"
"lint": "yarn lint:eslint # && yarn lint:types",
"lint:eslint": "eslint '**/*.{js,ts}'",
"lint:fix": "eslint --fix '**/*.{js,ts}'",
"lint:types": "tsc"
},
"dependencies": {
Copy link
Member

Choose a reason for hiding this comment

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

moving the "dependencies" section makes this diff hard to read.

"@agoric/ertp": "0.16.3-u13.0",
"@agoric/zoe": "0.26.3-u13.0",
Comment on lines +22 to +23
Copy link
Member

Choose a reason for hiding this comment

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

Have you checked that the resulting bundle is a reasonable size? that we have just 1 version of each endo package?

"@endo/far": "0.2.18",
"@endo/marshal": "0.8.5",
"@endo/patterns": "0.2.2"
},
"devDependencies": {
"agoric": "^0.21.2-u12.0",
"@agoric/deploy-script-support": "^0.10.4-u12.0",
"@endo/bundle-source": "^2.8.0",
"@agoric/deploy-script-support": "^0.10.4-u13.0",
"@agoric/eslint-config": "dev",
"@endo/bundle-source": "2.5.2-upstream-rollup",
"@endo/eslint-plugin": "^0.5.2",
"@endo/init": "^0.5.60",
"@endo/promise-kit": "0.2.56",
"@endo/ses-ava": "^0.2.44",
"@jessie.js/eslint-plugin": "^0.4.0",
"@typescript-eslint/eslint-plugin": "^6.7.0",
"@typescript-eslint/parser": "^6.7.0",
"agoric": "^0.22.0-u13.0",
"ava": "^5.3.0",
"eslint": "^8.47.0",
"eslint-config-airbnb-base": "^15.0.0",
Expand All @@ -41,20 +50,8 @@
"import-meta-resolve": "^2.2.1",
"prettier": "^3.0.3",
"prettier-plugin-jsdoc": "^1.0.0",
"type-coverage": "^2.26.3",
"typescript": "~5.2.2"
},
"resolutions": {
"@babel/code-frame": "7.18.6",
"@babel/highlight": "7.22.5"
},
"dependencies": {
"@agoric/zoe": "^0.26.3-u12.0",
"@agoric/ertp": "^0.16.3-u12.0",
"@endo/far": "^0.2.22",
"@endo/marshal": "^0.8.9",
"@endo/patterns": "^0.2.5"
},
"ava": {
"files": [
"test/**/test-*.js"
Expand All @@ -73,8 +70,45 @@
},
"homepage": "https://github.com/agoric-labs/dapp-join-game#readme",
"eslintConfig": {
"env": {
Copy link
Member

Choose a reason for hiding this comment

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

Whatever knowledge you have learned about lint config, would you please contribute it to...

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, let me gather some feedback here first to make sure the approach is correct. The main changes are -

  1. use typescript as the eslint parser
  2. add no-void and no-floating-promises rules
  3. reimplement _ignore patterns for unused-vars, as @typescript-eslint also checks for this (curious - can we dedupe no-unused-vars and @typescript-eslint/no-usused-vars. Both are effectively doing the same thing)?

Here would be the updated directions from the current answer:

  1. Run:
yarn add -D \
 eslint \
 @agoric/eslint-config@dev \
 @endo/eslint-plugin \
 @jessie.js/eslint-plugin \
  eslint-config-airbnb-base \
 eslint-plugin-jsdoc \
 eslint-config-prettier \
 eslint-plugin-import \
- eslint-plugin-github 
+ eslint-plugin-github \
+ @typescript-eslint/eslint-plugin
+ @typescript-eslint/parser
+ typescript
  1. Add the following to your package.json
 "eslintConfig" : {
+  "env": {
+    "node": true
+  },
+  "parser": "@typescript-eslint/parser",
   "parserOptions": {
-      "sourceType": "module",
-      "ecmaVersion": 6
+    "project": "./tsconfig.json",
+    "sourceType": "module",
+    "ecmaVersion": 2020
   },
   "extends": [
+    "plugin:@typescript-eslint/recommended",
     "@agoric"
   ],
+  "plugins": [
+    "@typescript-eslint",
+    "prettier"
+  ],
+  "rules": {
+    "@typescript-eslint/no-floating-promises": "warn",
+    "no-void": [
+      "error",
+      {
+        "allowAsStatement": true
+      }
+    ],
+    "prettier/prettier": "warn",
+    "@typescript-eslint/no-unused-vars": [
+      "error",
+      {
+        "vars": "all",
+        "args": "all",
+        "argsIgnorePattern": "^_",
+        "varsIgnorePattern": "^_"
+      }
+    ]
+  },
 }
  1. Add a tsconfig.json file (new)
+{
+  "compilerOptions": {
+    "noEmit": true,
+    "target": "esnext",
+    "module": "esnext",
+    "moduleResolution": "node",
+    "skipLibCheck": true,
+    "checkJs": false,
+    "allowJs": true,
+    "allowSyntheticDefaultImports": true,
+    "maxNodeModuleJsDepth": 2,
+    "strict": true
+  },
+  "include": ["**/*.js", "**/*.ts"],
+  "exclude": ["bundles"]
+}
  1. Run yarn eslint '**/*.{js,ts}' (or specify other files/directory globs). Run yarn tsc to lint types.

"node": true
},
"parser": "@typescript-eslint/parser",
"parserOptions": {
"project": "./tsconfig.json",
"sourceType": "module",
"ecmaVersion": 2020
},
"extends": [
"plugin:@typescript-eslint/recommended",
"@agoric"
],
"plugins": [
"@typescript-eslint",
"prettier"
],
"rules": {
"@typescript-eslint/prefer-ts-expect-error": "warn",
"@typescript-eslint/no-floating-promises": "warn",
"no-void": [
"error",
{
"allowAsStatement": true
}
],
"prettier/prettier": "warn",
"@typescript-eslint/no-unused-vars": [
"error",
{
"vars": "all",
"args": "all",
"argsIgnorePattern": "^_",
"varsIgnorePattern": "^_"
}
]
},
"ignorePatterns": [
"bundles"
]
},
"prettier": {
Expand Down
8 changes: 3 additions & 5 deletions contract/scripts/build-game1-start.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,9 @@ export const game1ProposalBuilder = async ({ publishRef, install }) => {
getManifestForGame1.name,
{
game1Ref: publishRef(
install(
'../src/gameAssetContract.js',
'../bundles/bundle-game1.js',
{ persist: true },
),
install('../src/gameAssetContract.js', '../bundles/bundle-game1.js', {
persist: true,
}),
),
},
],
Expand Down
3 changes: 2 additions & 1 deletion contract/src/gameAssetContract.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,9 @@ const bagValueSize = amt => {
return total;
};

/** @typedef {{joinPrice: Amount<AssetKind>}} GamePlacesTerms */
/**
* @param {ZCF<{joinPrice: Amount}>} zcf
* @param {ZCF<GamePlacesTerms>} zcf
*/
export const start = async zcf => {
const { joinPrice } = zcf.getTerms();
Expand Down
17 changes: 12 additions & 5 deletions contract/src/start-game1-proposal.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
import { E } from '@endo/far';
import { makeMarshal } from '@endo/marshal';
import { AmountMath } from '@agoric/ertp/src/amountMath.js';
import '@agoric/zoe/exported.js';
Copy link
Member

Choose a reason for hiding this comment

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

These bloat bundles. Ugh. Please add an XXX comment referring to...

important


console.warn('start-game1-proposal.js module evaluating');

Expand All @@ -26,6 +27,11 @@ const makeBoardAuxNode = async (chainStorage, boardId) => {
return E(boardAux).makeChildNode(boardId);
};

/**
* @param {ERef<StorageNode>} chainStorage
* @param {ERef<import('@agoric/vats/src/types').Board>} board
* @param {ERef<Brand>} brand
*/
const publishBrandInfo = async (chainStorage, board, brand) => {
const [id, displayInfo] = await Promise.all([
E(board).getId(brand),
Expand All @@ -38,28 +44,25 @@ const publishBrandInfo = async (chainStorage, board, brand) => {

/**
* Core eval script to start contract
*
* @param {BootstrapPowers} permittedPowers
* XXX FIXME File '~/agoric-sdk/packages/vats/src/core/types.js' is not a module
Copy link
Member

@turadg turadg Dec 30, 2023

Choose a reason for hiding this comment

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

* @param {import('@agoric/vats/src/core/types').BootstrapPowers} permittedPowers
*/
export const startGameContract = async permittedPowers => {
console.error('startGameContract()...');
const {
consume: { board, chainStorage, startUpgradable, zoe },
brand: {
consume: { IST: istBrandP },
// @ts-expect-error dynamic extension to promise space
produce: { Place: producePlaceBrand },
},
issuer: {
consume: { IST: istIssuerP },
// @ts-expect-error dynamic extension to promise space
produce: { Place: producePlaceIssuer },
},
installation: {
consume: { game1: game1InstallationP },
},
instance: {
// @ts-expect-error dynamic extension to promise space
produce: { game1: produceInstance },
},
} = permittedPowers;
Expand Down Expand Up @@ -117,6 +120,10 @@ const gameManifest = {
};
harden(gameManifest);

/**
* @param {{restoreRef: (ref: unknown) => Promise<unknown> }} r
* @param {{ game1Ref: unknown }} g
*/
export const getManifestForGame1 = ({ restoreRef }, { game1Ref }) => {
return harden({
manifest: gameManifest,
Expand Down
2 changes: 1 addition & 1 deletion contract/test/mintStable.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ const centralSupplyPath = myRequire.resolve(
* just for this purpose. Each time we want to mint a fee/stable token payment,
* we make an instance of of the `centralSupply` contract.
*
* @param {Object} powers
* @param {object} powers
* @param {ERef<FeeMintAccess>} powers.feeMintAccess for minting IST
* @param {ERef<ZoeService>} powers.zoe for starting the `centralSupply` contract
* @param {BundleCache} powers.bundleCache for bundling the `centralSupply` contract
Expand Down
15 changes: 12 additions & 3 deletions contract/test/test-contract.js
Original file line number Diff line number Diff line change
Expand Up @@ -78,8 +78,9 @@ test('Start the contract', async t => {
*
* @param {import('ava').ExecutionContext} t
* @param {ZoeService} zoe
* @param {ERef<import('@agoric/zoe/src/zoeService/utils').Instance<GameContractFn>} instance
* @param {Purse} purse
* @param {import('@agoric/zoe/src/zoeService/utils').Instance<GameContractFn>} instance
* @param {Purse<"nat">} purse
0xpatrickdev marked this conversation as resolved.
Show resolved Hide resolved
* @param {string[]} choices
*/
const alice = async (
t,
Expand All @@ -89,7 +90,7 @@ const alice = async (
choices = ['Park Place', 'Boardwalk'],
) => {
const publicFacet = E(zoe).getPublicFacet(instance);
// @ts-expect-error Promise<Instance> seems to work
/** @type {import('../src/gameAssetContract.js').GamePlacesTerms} */
const terms = await E(zoe).getTerms(instance);
const { issuers, brands, joinPrice } = terms;

Expand Down Expand Up @@ -187,6 +188,14 @@ test('use the code that will go on chain to start the contract', async t => {
});

const { zoe } = t.context;
/**
* @param {{
* installation: ERef<Installation<GameContractFn>>;
Copy link
Member

Choose a reason for hiding this comment

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

GameContractFn seems overly specific here. But perhaps the relevant generic type is a pain.

* issuerKeywordRecord: IssuerKeywordRecord;
* label: string;
* terms: import('../src/gameAssetContract.js').GamePlacesTerms;
* }} opts
*/
const startUpgradable = async ({
installation,
issuerKeywordRecord,
Expand Down
18 changes: 18 additions & 0 deletions contract/tsconfig.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
{
"compilerOptions": {
"noEmit": true,
"target": "esnext",
"module": "esnext",
"moduleResolution": "node",
"skipLibCheck": true,
// Don't check by default because that includes node_modules in JS
"checkJs": false,
// Features implied with jsconfig.json that have to be explicit in tsconfig.json
"allowJs": true,
"allowSyntheticDefaultImports": true,
"maxNodeModuleJsDepth": 2,
"strict": true
},
"include": ["**/*.js", "**/*.ts"],
"exclude": ["bundles"]
}
4 changes: 0 additions & 4 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,6 @@
"contract",
"ui"
],
"resolutions": {
"@babel/code-frame": "7.18.6",
"@babel/highlight": "7.22.5"
},
"scripts": {
"start:docker": "cd contract && docker compose up -d",
"docker:logs": "cd contract; docker compose logs --tail 200 -f",
Expand Down
4 changes: 3 additions & 1 deletion ui/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@
},
"resolutions": {
"**/ses": "^0.18.8",
"**/@agoric/xsnap": "0.14.3-dev-9f085d3.0"
"**/@agoric/xsnap": "0.14.3-dev-9f085d3.0",
"@babel/code-frame": "7.18.6",
"@babel/highlight": "7.22.5"
}
}
Loading