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
Changes from 1 commit
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
Prev Previous commit
Next Next commit
feat(contract): typedef improvements
  • Loading branch information
0xpatrickdev committed Dec 30, 2023
commit f2d6dfc9759d4740eadd807ece2692a7d941a523
3 changes: 2 additions & 1 deletion contract/src/gameAssetContract.js
Original file line number Diff line number Diff line change
@@ -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();
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');

@@ -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),
@@ -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;
@@ -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,
2 changes: 1 addition & 1 deletion contract/test/mintStable.js
Original file line number Diff line number Diff line change
@@ -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);
3 changes: 2 additions & 1 deletion contract/test/test-bundle-source.js
Original file line number Diff line number Diff line change
@@ -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);
17 changes: 13 additions & 4 deletions contract/test/test-contract.js
Original file line number Diff line number Diff line change
@@ -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';
@@ -78,8 +79,8 @@ 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 (
@@ -90,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;

@@ -188,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,