Contracts SATP compatability #427

nksazonov · 2022-03-29T12:34:20Z

Contracts changes:

appDefinition and challengeDuration are now used in channelId computations (which includes chainID, participants, channelNonce, appDefinition, challengeDuration)
State struct representation has changed: bytes appPart and bytes outcome instead of bytes32 appPartHash and bytes32 outcomeHash
in ForceMove State is encoded in the same way as in go-nitro - not as a struct, but as a set of State's fields. Also order of arguments has changed.
function signatures, EmbeddedApplication.sol were updated to support changes above
additional step of decoding added to CountingApp to correspond to go-nitro
tests updated to support changes above. Also some ts types fixed and imports updates to support newer packages versions

TODO:

change validTransition to latestSupportedState in ForceMoveApp.sol
update function signatures and tests to support that update

nksazonov · 2022-04-01T08:35:48Z

Trying to clarify what is need to be changed related to latestSupportedState implementation. As I understand from SATP paper, all state transition checks should be done in it in ForceMoveApp. If so, should latestSupportedState replace _requireStateSupportedBy logic currently implemented in ForceMove?

andrewgordstewart · 2022-04-01T15:15:29Z

As I understand from SATP paper, all state transition checks should be done in it in ForceMoveApp. If so, should latestSupportedState replace _requireStateSupportedBy logic currently implemented in ForceMove?

Yes, that's correct.

In the current nitro-protocol, the adjudicator holds a strong opinion about what makes a state "supported". "Supported" should mean: the adjudicator is willing

SATP suggests delegating that responsibility latestSupportedState. This removes a guard rail built into the protocol, but is a lot more flexible. We can extract the current code into a library that different latestSupportedState implementations can use, if that's useful.

As an, imagine a 3-party channel between Alice, Irene and Bob. The current rules say that at any point in time, either Alice can force an update on chain or Bob can; it's not possible to allow both of them to force an update (which is useful in a channel when Alice and Bob are trading between themselves, as opposed to Alice always giving assets to Bob).

Here's an example of how these kinds of rules might be written (replace validSupport with latestSupportedState). Is this example clear? (You can leave comments here if you like!)

nksazonov · 2022-04-04T15:48:27Z

The example you have sent is pretty clear, thanks.

geoknee

Thanks for this @nksazonov! I'm still working through the changes but thought it was a good idea to share a couple of points early on :-)

geoknee · 2022-04-04T17:19:06Z

nitro-protocol/contracts/libraries/ECRecovery.sol

+//SPDX-License-Identifier: MIT License
+pragma solidity 0.7.4;
+
+library ECRecovery {
+
+  /**
+   * @dev Recover signer address from a message by using his signature
+   * @param hash bytes32 message, the hash is the signed message. What is recovered is the signer address.
+   * @param v v part of a signature
+   * @param r r part of a signature
+   * @param s s part of a signature
+   */
+  function recover(bytes32 hash, uint8 v, bytes32 r, bytes32 s) internal pure returns (address) {
+
+    // Version of signature should be 27 or 28, but 0 and 1 are also possible versions
+    if (v < 27) {
+      v += 27;
+    }
+
+    // If the version is correct return the signer address
+    if (v != 27 && v != 28) {
+      return (address(0));
+    } else {
+      return ecrecover(hash, v, r, s);
+    }
+  }
+
+}


This file is new, but doesn't appear to be used anywhere?

It looks to me like a wrapper for ecrecover which may add some extra safety. Is that right? Is there something equivalent in the openzeppelin library that we could use https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/cryptography/ECDSA.sol ?

Edit: I see that it is used inside ForceMove.sol (didn't spot it because GitHub hides large diffs!). Still I would like to understand the thinking behind introducing it.

It is a wrapper for ecrecover indeed. The reason this file was introduced is inconsistency of returned v signature value in go-ethereum (as described here) and its difference with what pure ecrecover solidity function expects (i.e. v to be either 27 or 28).

We thought of using fault-proof OpenZeppelin analogy, but it has a lot of unnecessary tweaks and, more importantly, requires modification to support what we need. Thus it was simpler and clearer to make a library with the same modifications that should have been done to OpenZeppelin contract.

I see. I wonder if it is better to fix this problem in the go-nitro client code, rather than on chain? That seems to be the advice from the go-ethereum team. What do you think?

I agree. We will try to fix it from go-nitro client code ASAP.

nitro-protocol/contracts/interfaces/IForceMove.sol

nitro-protocol/test/test-helpers.test.ts

geoknee · 2022-04-05T10:10:51Z

nitro-protocol/tsconfig.json

-    "types": ["node", "jest"],
+    "target": "es2018",
+    "module": "commonjs",
+    "strict": true,


Good to see "strict": true. Does tsc . work, or are there errors?

tsc --project ./ from root folder produces

Errors Files 7 node_modules/@types/jest/index.d.ts:35 2 scripts/postdeploy.ts:6

Maybe outside the scope of this work, but it might be nice to get to zero errors. Also, we want to gitignore the resulting dist folder.

geoknee · 2022-04-05T10:18:29Z

nitro-protocol/test/contracts/ForceMove/challenge.test.ts

+    await (await ForceMove.setStatus(getChannelId(twoPartyFixedPart), HashZero)).wait();
  });
+  // FIX: even if dropping channel status before each test, turn nums from prev tests are saved and can cause reverts
  it.each`
    description                                     | appData   | outcome                            | turnNums  | challenger
    ${'challenge(0,1) accepted'}                    | ${[0, 0]} | ${[]}                              | ${[0, 1]} | ${1}
    ${'challenge(1,2) accepted'}                    | ${[0, 0]} | ${[]}                              | ${[1, 2]} | ${0}
-    ${'challenge(1,2) accepted, MAX_OUTCOME_ITEMS'} | ${[0, 0]} | ${largeOutcome(MAX_OUTCOME_ITEMS)} | ${[1, 2]} | ${0}
+    ${'challenge(3,4) accepted, MAX_OUTCOME_ITEMS'} | ${[0, 1]} | ${largeOutcome(MAX_OUTCOME_ITEMS)} | ${[3, 4]} | ${0}


I'd like to understand this a little better. What's going wrong?

The last test fails due to transaction revert with reason turnNumRecord not increased, which corresponds to CountingApp.sol. Both changing turnNums to [3, 4] and removing middle test help to avoid this revert. Unfortunately, I was not able to debug CountingApp.sol (via console.log) behavior with this tests because ganache is created dynamically and even setting env args to enable logging does not help.

nitro-protocol/src/transactions.ts

geoknee · 2022-04-05T17:51:18Z

nitro-protocol/contracts/interfaces/IForceMoveApp.sol

+/**
+ * @dev The IForceMoveApp interface calls for its children to implement an application-specific validTransition function, defining the state machine of a ForceMove state channel DApp.
+ */
+interface IForceMoveApp {
+    struct VariablePart {
+        bytes outcome;
+        bytes appData;
+    }


@nksazonov This struct needs updating, too, I think?

Take a look at 744dbbe and 3b4bfba

Yeah, I've also realized it needs updating to support latestSupportedState.

geoknee · 2022-04-06T17:03:39Z

@nksazonov thanks a lot for all these contributions! We would like to propose the following workflow:

We will merge this PR as is very soon.
We will make issues with the tag "on-chain" to manage any immediate issues arising from this PR (i.e. the comments above).
We will add you and any other openware devs as external collaborators so you can make branches and raise pull requests in this repo.
We can assign issues to prevent duplicating work.
We will setup continuous integration (see Add node.js github action #487) as soon as possible. One of the outstanding pieces of work will be to get the tests passing 100% (I think we are very close).
We may add some other CI checks such as linting, prettier and so on.

How does this sound to you? Please let me know your thoughts.

geoknee · 2022-04-06T17:16:47Z

There is also another thing I would like to resolve before merging!

We need to resolve an issue with the readme files. Most of our team use Mac OSX, which has a case insensitive file system. When I check out this branch on a Mac, I immediately see some unstaged changes to ./nitro-protocol/README.md. These changes are reverting the readme back to the old nitro-protocol readme from the statechannels monorepo. If I restore the file (reject the changes), I immediately get some unstaged changes to ./nitro-protocol/readme.md, changing that file to your new readme. It's hard to tell on my system, but I guess there are two files there which differ only in the casing? In that case, deleting one of them ought to solve the problem?

nksazonov · 2022-04-06T17:19:31Z

How does this sound to you? Please let me know your thoughts.

@geoknee sounds like an amazing collaboration start!
Before merging I propose we resolve all minor problems we have discussed (e.g. comments and gas limit).

geoknee · 2022-04-06T17:20:21Z

Before merging I propose we resolve all minor problems we have discussed (e.g. comments and gas limit).

Sounds good to me!

nksazonov · 2022-04-06T17:24:30Z

It's hard to tell on my system, but I guess there are two files there which differ only in the casing?

Yes, this is the reason. Deleting ours should solve the problem.

nksazonov force-pushed the feature/contracts-satp branch 3 times, most recently from 5bc8660 to e378615 Compare March 29, 2022 17:27

nksazonov added 2 commits March 29, 2022 20:29

add codebase

28229ec

fea: change state sig, encoding, migrate tests

e378615

andrewgordstewart requested review from andrewgordstewart and geoknee March 31, 2022 21:03

geoknee reviewed Apr 4, 2022

View reviewed changes

This was referenced Apr 4, 2022

Run nitro-protocol tests in CI #485

Closed

Test go-nitro's state.Sign against nitro-protocol contracts #486

Closed

geoknee added the on-chain label Apr 5, 2022

geoknee reviewed Apr 5, 2022

View reviewed changes

nitro-protocol/test/test-helpers.test.ts Outdated Show resolved Hide resolved

geoknee reviewed Apr 5, 2022

View reviewed changes

nitro-protocol/src/transactions.ts Outdated Show resolved Hide resolved

geoknee reviewed Apr 5, 2022

View reviewed changes

nksazonov added 2 commits April 6, 2022 20:30

remove duplicate readme

2a5c71d

ref: resolve minor problems

aa29a1e

geoknee approved these changes Apr 7, 2022

View reviewed changes

geoknee merged commit 3956d5a into statechannels:main Apr 7, 2022

This was referenced Apr 7, 2022

Migrate from validTransition to latestSupportedState #496

Closed

Revert on-chain signature recovery to plain ecrecover #497

Closed

This was referenced Apr 7, 2022

Add tsc to continuous integration checks (and fix all errors) #498

Closed

Update VariablePart to match off-chain code #499

Closed

calj deleted the feature/contracts-satp branch April 13, 2022 04:53

geoknee mentioned this pull request Apr 13, 2022

Update VariablePart to match off-chain code #508

Merged

15 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Contracts SATP compatability #427

Contracts SATP compatability #427

nksazonov commented Mar 29, 2022 •

edited

Loading

nksazonov commented Apr 1, 2022

andrewgordstewart commented Apr 1, 2022

nksazonov commented Apr 4, 2022

geoknee left a comment

geoknee Apr 4, 2022

geoknee Apr 4, 2022

nksazonov Apr 5, 2022

geoknee Apr 5, 2022

nksazonov Apr 5, 2022

geoknee Apr 5, 2022

nksazonov Apr 5, 2022

geoknee Apr 5, 2022

geoknee Apr 5, 2022

nksazonov Apr 5, 2022

geoknee Apr 5, 2022

nksazonov Apr 5, 2022

geoknee commented Apr 6, 2022

geoknee commented Apr 6, 2022

nksazonov commented Apr 6, 2022

geoknee commented Apr 6, 2022

nksazonov commented Apr 6, 2022

Contracts SATP compatability #427

Contracts SATP compatability #427

Conversation

nksazonov commented Mar 29, 2022 • edited Loading

nksazonov commented Apr 1, 2022

andrewgordstewart commented Apr 1, 2022

nksazonov commented Apr 4, 2022

geoknee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geoknee commented Apr 6, 2022

geoknee commented Apr 6, 2022

nksazonov commented Apr 6, 2022

geoknee commented Apr 6, 2022

nksazonov commented Apr 6, 2022

nksazonov commented Mar 29, 2022 •

edited

Loading