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 4 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
53 changes: 45 additions & 8 deletions contract/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -13,21 +13,24 @@
"start": "yarn docker:make clean start-contract print-key",
"build": "exit 0",
"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": "eslint '**/*.{js,ts}'",
"lint-fix": "eslint --fix '**/*.{js,ts}'",
"lint-fix-jessie": "eslint -c '.eslintrc-jessie.js' --fix '**/*.js'",
"lint-jessie": "eslint -c '.eslintrc-jessie.js' '**/*.js'",
"typecheck": "tsc"
Copy link
Member

Choose a reason for hiding this comment

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

not critical:

elsewhere this is called lint:types and we have lint:eslint and lint does both.

see the PR linked from Agoric/agoric-sdk#8274 (reply in thread)

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree, didn't want to change lint-* as a part of this, but agree it should standardized.

Copy link
Member Author

Choose a reason for hiding this comment

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

see 3ce6458

},
"devDependencies": {
"agoric": "^0.21.2-u12.0",
"@agoric/deploy-script-support": "^0.10.4-u12.0",
"@agoric/eslint-config": "dev",
"@endo/bundle-source": "^2.8.0",
"@endo/eslint-plugin": "^0.5.2",
"@endo/init": "^0.5.60",
"@endo/promise-kit": "^1.0.1",
Copy link
Member

Choose a reason for hiding this comment

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

important
That version smells like trouble. We need to keep the endo versions consistent, and I'm pretty sure this dapp is not up to the 1.x stuff yet.

cf

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. Updated to 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.21.2-u12.0",
"ava": "^5.3.0",
"eslint": "^8.47.0",
"eslint-config-airbnb-base": "^15.0.0",
Expand All @@ -49,8 +52,8 @@
"@babel/highlight": "7.22.5"
},
"dependencies": {
"@agoric/zoe": "^0.26.3-u12.0",
"@agoric/ertp": "^0.16.3-u12.0",
"@agoric/zoe": "^0.26.3-u12.0",
"@endo/far": "^0.2.22",
"@endo/marshal": "^0.8.9",
"@endo/patterns": "^0.2.5"
Expand All @@ -73,13 +76,47 @@
},
"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": "^_"
}
]
}
},
"prettier": {
"trailingComma": "all",
"arrowParens": "avoid",
"singleQuote": true
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

no newline at end?

Copy link
Member Author

Choose a reason for hiding this comment

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

Prettier mishap, should be fixed now

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 {StandardTerms & { joinPrice: Amount<'nat'> }} GamePlacesTerms */
Copy link
Member

Choose a reason for hiding this comment

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

important: Why constrain it to nat amounts?

StandardTerms is redundant here. The parameter to the ZCF<...> type is custom terms.

Copy link
Member Author

Choose a reason for hiding this comment

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

StandardTerms is redundant here. The parameter to the ZCF<...> type is custom terms.

Agree/ack. This was added in an attempt to get const terms = await E(zoe).getTerms(instance); inside test-contract.js to not complain Property 'brands' does not exist on type 'GamePlacesTerms'..

Open to hints for getting StandardTerms recognized properly in the testing context.

/**
* @param {ZCF<{joinPrice: Amount}>} zcf
* @param {ZCF<GamePlacesTerms>} zcf
*/
export const start = async zcf => {
const { joinPrice } = zcf.getTerms();
Expand Down
18 changes: 12 additions & 6 deletions contract/src/start-game1-proposal.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
// @ts-check
import { E } from '@endo/far';
import { E } from '@endo/eventual-send';
Copy link
Member

Choose a reason for hiding this comment

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

@endo/far is the norm / convention we want to teach. Please revert.
important

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,9 @@ const gameManifest = {
};
harden(gameManifest);

/**
* @param {{restoreRef: (ref: unknown) => Promise<unknown> }} r
* @param {{ game1Ref: unknown }} g */
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @param {{ game1Ref: unknown }} g */
* @param {{ game1Ref: unknown }} g
*/

export const getManifestForGame1 = ({ restoreRef }, { game1Ref }) => {
return harden({
manifest: gameManifest,
Expand Down
4 changes: 2 additions & 2 deletions contract/test/mintStable.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
*/

// @ts-check
import { E } from '@endo/far';
import { E } from '@endo/eventual-send';
Copy link
Member

Choose a reason for hiding this comment

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

as before, no, please.

import { createRequire } from 'module';

const myRequire = createRequire(import.meta.url);
Expand All @@ -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
3 changes: 2 additions & 1 deletion contract/test/test-bundle-source.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@ import { test } from './prepare-test-env-ava.js';

import { createRequire } from 'module';
import bundleSource from '@endo/bundle-source';
import { E, passStyleOf } from '@endo/far';
import { E } from '@endo/eventual-send';
import { passStyleOf } from '@endo/far';
Copy link
Member

Choose a reason for hiding this comment

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

pls revert

Copy link
Member Author

Choose a reason for hiding this comment

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

This was added since anytime E is imported from @endo/far, instead of @endo/eventual-send, typescript will complain that This expression is not callable. Type 'never' has no call signatures
image

Copy link
Member

Choose a reason for hiding this comment

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

Sounds like a bug in @endo/far's type exports. Hopefully fixed in subsequent releases.

Either way, eventual-send is the source: https://github.com/endojs/endo/blob/2179108cb9247e9499c1f319de907d7cd365f314/packages/far/src/index.js#L1

@endo/far is just for convience. https://github.com/endojs/endo/blob/master/packages/far/README.md#L6

Copy link
Member

Choose a reason for hiding this comment

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

as noted above, @endo/far is the norm / convention. It started a couple years ago. I'm not in a position to approve a migration away from it on my own.

Copy link
Member Author

Choose a reason for hiding this comment

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

Tracking this here #31 to keep this PR moving

import { makeZoeKitForTest } from '@agoric/zoe/tools/setup-zoe.js';

const myRequire = createRequire(import.meta.url);
Expand Down
18 changes: 14 additions & 4 deletions contract/test/test-contract.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@
import { test as anyTest } from './prepare-test-env-ava.js';

import { createRequire } from 'module';
import { E, Far } from '@endo/far';
import { E } from '@endo/eventual-send';
import { Far } from '@endo/far';
import { makePromiseKit } from '@endo/promise-kit';
0xpatrickdev marked this conversation as resolved.
Show resolved Hide resolved
import { makeCopyBag } from '@endo/patterns';
import { makeNodeBundleCache } from '@endo/bundle-source/cache.js';
Expand Down Expand Up @@ -78,8 +79,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 +91,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 +189,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
17 changes: 17 additions & 0 deletions contract/tsconfig.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
{
"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"]
}
24 changes: 24 additions & 0 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,11 @@
"@endo/nat" "4.1.27"
"@endo/promise-kit" "0.2.56"

"@agoric/eslint-config@dev":
version "0.4.1-dev-a0d8229.0"
resolved "https://registry.yarnpkg.com/@agoric/eslint-config/-/eslint-config-0.4.1-dev-a0d8229.0.tgz#cbe1cbd0f39bf65bfbc0d2fdf93fa8f0ae4dbbdf"
integrity sha512-cs1Gr4WFm2vY5ixBTP0YYYE1503x5wSDYPZuvjyVvCTs+6eRGj7cKOd+1qkZ+uRSFlOoKFYMaQHmqvAuzXMtAg==

"@agoric/eventual-send@^0.14.1":
version "0.14.1"
resolved "https://registry.yarnpkg.com/@agoric/eventual-send/-/eventual-send-0.14.1.tgz#b414888bed67cf003a61bd22da30a70f79b8f9dc"
Expand Down Expand Up @@ -1401,6 +1406,11 @@
resolved "https://registry.yarnpkg.com/@endo/env-options/-/env-options-0.1.4.tgz#e516bc3864f00b154944e444fb8996a9a0c23a45"
integrity sha512-Ol8ct0aW8VK1ZaqntnUJfrYT59P6Xn36XPbHzkqQhsYkpudKDn5ILYEwGmSO/Ff+XJjv/pReNI0lhOyyrDa9mg==

"@endo/env-options@^1.0.1":
version "1.0.1"
resolved "https://registry.yarnpkg.com/@endo/env-options/-/env-options-1.0.1.tgz#a6ad1951f3303426cd15956aa7b95ea06cb34ad0"
integrity sha512-5hieu6ow9Kgf2wKKchE1xQEN7VlKVLL3O0eEjxN9d52XodHMbEFu0gGoFA6NeQJq9SHNrbgJhZDfMkPaHvoFxg==

"@endo/eslint-plugin@^0.5.2":
version "0.5.2"
resolved "https://registry.yarnpkg.com/@endo/eslint-plugin/-/eslint-plugin-0.5.2.tgz#835d22e9ff17d9935f7f565e50a21ef07aa92ca2"
Expand Down Expand Up @@ -1603,6 +1613,13 @@
dependencies:
ses "^0.18.8"

"@endo/promise-kit@^1.0.1":
version "1.0.1"
resolved "https://registry.yarnpkg.com/@endo/promise-kit/-/promise-kit-1.0.1.tgz#056c8cd59e52260fc4cea2a83422450333a04e26"
integrity sha512-JLhfkuQaERVvf+G+kXpDjyzyik3kxkX1FHbiDQfK2ge9Ltz7vRk/OD+yZma0UE2Rn91F2B/mvelxz1MxlYibwA==
dependencies:
ses "^1.0.1"

"@endo/[email protected]":
version "0.2.40"
resolved "https://registry.yarnpkg.com/@endo/ses-ava/-/ses-ava-0.2.40.tgz#8a6c1f668131ecbe4d06339cac2a8346253089b8"
Expand Down Expand Up @@ -6168,6 +6185,13 @@ ses@^0.18.4, ses@^0.18.5, ses@^0.18.8:
dependencies:
"@endo/env-options" "^0.1.4"

ses@^1.0.1:
Copy link
Member

Choose a reason for hiding this comment

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

more problematic endo/ses versions

version "1.0.1"
resolved "https://registry.yarnpkg.com/ses/-/ses-1.0.1.tgz#ede4c688b150ffa4d368679d8c1f8ee57d9e4836"
integrity sha512-iuqGQ1dsktipGgmC/FLL2agq88WcarXxbFDKMEPVyRUxhAhReKpBIO63uDK4IvYYofFJ6FpfhNn/mVTNZpPOCg==
dependencies:
"@endo/env-options" "^1.0.1"

set-function-length@^1.1.1:
version "1.1.1"
resolved "https://registry.yarnpkg.com/set-function-length/-/set-function-length-1.1.1.tgz#4bc39fafb0307224a33e106a7d35ca1218d659ed"
Expand Down