Lint rules for contracts

[turadg]

What is the Problem Being Solved?

Our contracts need tighter linting than our general eslint-config. @jessie-check provides some of that additional constraint.

TBD what else we need to constrain (ticket created with the idea that jessie-check wasn't appropriate but it is in part)

Description of the Design

Define file path patterns for what needs to be contract-linted so that files don't have to include a pragma.

Define rules for that set

Consider using rules that require type information from TypeScript (using https://typescript-eslint.io/ )

Security Considerations

--

Test Plan

--

[turadg]

#3787 will introduce some constraints on use of promises in contracts.
A promise in a contract that needs to survive an upgrade will not be able to use .then. All of RAM goes away during upgrade. await is an implicit then with a postponed callback.

Because upgrades are voluntary we can wait to quiesce non-durable promises. But we also need to think through unexpected crashing.

"All contracts should be of the form that awaits inside of a contract are not awaiting someone else's action." - @dtribble

[Chris-Hibbert]

How hard would it to detect files with a zoe-compliant start() function?

[turadg]

How hard would it to detect files with a zoe-compliant start() function?

I think the rule would have a precondition that it searches the file for whether to try testing it. Like https://github.com/Agoric/agoric-sdk/blob/fd1c24a84584f6b5f7b7d5e8b21d756464db05b6/packages/eslint-plugin/lib/processors/use-jessie.js (btw @michaelfig where does that code live now? I can't find a repo for https://www.npmjs.com/package/@jessie.js/eslint-plugin )

I imagine you can get an AST for that first pass and skip if there's no start function exported. I don't think you'd be able to check much more of "zoe-compliant" with some Typescript annotations.

Though if the inclusion criterion was being zoe-compliant then it wouldn't be feasible to lint for that.

I take it you're asking because you'd prefer not to have a contract signifier in the filename. If so, what are some reasons?

[Chris-Hibbert]

just long names. I don't think it's a huge deal, I just thought it might not be hard to find the files without a naming convention.

[dckc]

Because @jessie-check errors on more than we want ...

Is that so? Please elaborate.

[erights]

Because @jessie-check errors on more than we want ...

Is that so? Please elaborate.

I think I remember the issue: The Jessie definition serves several purposes. One of them is that Jessie be simple enough that a eval/apply style Jessie interpreter can be straightforwardly implemented in any conventional imperative language. In practice, this aligns extraordinarily well with Jessie's other design constraints, such as being simple enough that you (and eventually tools) can reason about your Jessie code. Our contracts need the second but not really the first in any deployment scenario we are considering.

Generators with yield cannot be implemented straightforwardly by an eval/apply style interpreter in a language without something like it. Except by doing a CPS transform, which is too much. Originally, async functions with await were also omitted for this reason. But our top-level-await restriction solved both simplicity needs (interpreter, reasoning). A top-level yield restriction would also solve the interpreter issue, but would be incompatible with the way yield is (and should be) used.

But generators with non-top-level yield are not dangerous or hard to reason about. So if we wanted to go out of our way to enable contracts to use them without warning, we could. OTOH, IMO it is adequate to do an @eslint-disable for those rare times this is an issue, or to quarantine these generators into separate modules without an @jessie-check.

All that said, I think contract specific lint rules would be great! Leaving aside generators, these should be in addition to Jessie rules, not instead of them.

Are there other cases that are problematic in this way other than generators?

[turadg]

https://github.com/Agoric/agoric-sdk/tree/ta/jessie-check-contracts adds jessie-check to all non-test contracts (that I could find use const start = )

/opt/agoric/agoric-sdk/packages/deploy-script-support/src/endo-pieces-contract.js
  11:23  error  'new' is not allowed in Jessie; use a 'maker' function  no-restricted-syntax
  24:27  error  'new' is not allowed in Jessie; use a 'maker' function  no-restricted-syntax
  40:11  error  'new' is not allowed in Jessie; use a 'maker' function  no-restricted-syntax
  60:24  error  'new' is not allowed in Jessie; use a 'maker' function  no-restricted-syntax

/opt/agoric/agoric-sdk/packages/run-protocol/src/reserve/assetReserve.js
  106:9   error  arbitrary computed property names are not allowed in Jessie; use leading '+'  no-restricted-syntax
  110:13  error  'new' is not allowed in Jessie; use a 'maker' function                        no-restricted-syntax

/opt/agoric/agoric-sdk/packages/run-protocol/src/vaultFactory/liquidateIncrementally.js
  149:3   error  generators are not allowed in Jessie                      no-restricted-syntax
  163:3   error  generators are not allowed in Jessie                      no-restricted-syntax
  174:7   error  Unexpected `await` expression (not top of function body)  @jessie.js/no-nested-await
  177:67  error  Unexpected `await` expression (not top of function body)  @jessie.js/no-nested-await
  201:27  error  Unexpected `await` expression (not top of function body)  @jessie.js/no-nested-await
  266:7   error  Unexpected `await` expression (not top of function body)  @jessie.js/no-nested-await

/opt/agoric/agoric-sdk/packages/zoe/src/contracts/autoswap.js
  74:26  error  'new' is not allowed in Jessie; use a 'maker' function  no-restricted-syntax

/opt/agoric/agoric-sdk/packages/zoe/src/contracts/barterExchange.js
  35:32  error  'new' is not allowed in Jessie; use a 'maker' function  no-restricted-syntax

/opt/agoric/agoric-sdk/packages/zoe/src/contracts/callSpread/pricedCallSpread.js
  105:15  error  Unexpected `await` expression (not top of function body)  @jessie.js/no-nested-await

/opt/agoric/agoric-sdk/packages/zoe/src/contracts/oracle.js
  71:40  error  Unexpected `await` expression (not top of function body)  @jessie.js/no-nested-await
  87:42  error  Unexpected `await` expression (not top of function body)  @jessie.js/no-nested-await

/opt/agoric/agoric-sdk/packages/zoe/src/contracts/priceAggregator.js
   81:25  error  'new' is not allowed in Jessie; use a 'maker' function  no-restricted-syntax
  409:19  error  'new' is not allowed in Jessie; use a 'maker' function  no-restricted-syntax

/opt/agoric/agoric-sdk/packages/zoe/src/contracts/priceAggregatorChainlink.js
  298:25  error  Expected { after 'if' condition                         curly
  351:30  error  Expected { after 'if' condition                         curly
  354:7   error  Expected { after 'if' condition                         curly
  418:7   error  Expected { after 'if' condition                         curly
  469:31  error  Expected { after 'if' condition                         curly
  470:35  error  Expected { after 'if' condition                         curly
  472:7   error  Expected { after 'if' condition                         curly
  474:7   error  Expected { after 'if' condition                         curly
  480:7   error  Expected { after 'if' condition                         curly
  482:7   error  Expected { after 'if' condition                         curly
  614:19  error  'new' is not allowed in Jessie; use a 'maker' function  no-restricted-syntax

/opt/agoric/agoric-sdk/packages/zoe/src/contracts/sellItems.js
  94:28  error  'new' is not allowed in Jessie; use a 'maker' function  no-restricted-syntax

/opt/agoric/agoric-sdk/packages/zoe/src/contracts/simpleExchange.js
  131:7  error  'new' is not allowed in Jessie; use a 'maker' function  no-restricted-syntax

Aside from the generator/await (which the last comment provides a solution for) the rest seem trivial to fix. Though I don't know the safe alternative to new Map().

All that said, I think contract specific lint rules would be great! Leaving aside generators, these should be in addition to Jessie rules, not instead of them.

👍 I'll revise the PR description.

[dckc]

Though I don't know the safe alternative to new Map().

import { makeMap } from 'jessie.js';

https://github.com/endojs/Jessie/blob/7cdc722ab7259347c1d7e55606b9fd660ac75549/packages/stdlib/src/ring0/makers.js#L14

[Tartuffo]

@turadg really needed for launch?

[dckc]

a wish for a contract static check: #7771 (comment)