-
Notifications
You must be signed in to change notification settings - Fork 44
[all] Introduction of AppRegistry and significant code improvements to facilitate it #163
Conversation
Tests currently pass for me with the recent changes but there are some caveats...
Still would appreciate any extra feedback @IIIIllllIIIIllllIIIIllllIIIIllllIIIIll |
69ef295
to
38a85b6
Compare
-1 on the name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM other than existing comments and the FIXME
@IIIIllllIIIIllllIIIIllllIIIIllllIIIIll I prefer "adjudicator" to "registry" tbh. It describes the raison d'etre of the contract, which is to adjudicate disputes on-chain. |
@mvanderh that explanation doesn't support the |
bef7fc9
to
087bfc9
Compare
@IIIIllllIIIIllllIIIIllllIIIIllllIIIIll Why not? The contract adjudicates app instances. |
@snario Status update? |
In progress. |
0beb011
to
1cbb61d
Compare
bee407c
to
f595e92
Compare
* Replace all ethers utils with module imports * Fixed a few places I missed the first time * Update rollup for cf.js to include ethers.constants
Co-Authored-By: snario <[email protected]>
fb.setState({ ...currentState, aliceBalance, bobBalance }) | ||
); | ||
|
||
return new StateChannel( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't feel right to me that we return new instances of StateChannel
objects from each of these methods. It seems like the StateChannel
object reference should be stable across all these operations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean that the class should not be immutable or that the implementation should hide the fact that it is immutable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@snario It depends. It could just be a naming issue. Are these objects purely internal to the machine or are they vended out to the Node / Playground Server as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the moment, purely internal. However, the intention is to use them in the Node
. It is unclear how they will be distributed. My recommendation is to make the machine
a subfolder of Node
and hoist the models
folder up into the node
source code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@snario Then I think they should probably be like how CF.js's AppInstance
objects work in that state channels are singletons. API consumers only see one consistent state channel object.
dcf979d
to
93b78ea
Compare
…o facilitate it (#163) * wip * wip * passing tests2 * apps folder tests pass * lint * cleaner code * cleaner code * install protocol tests * set state tests * test improvements * setup protocol tests * some fixes * tests pass for all cf ops * touch ups * code cleanup * start of cleaning up proposer tests * update waffle * clean up tests for install proposer * continue to ensure tests pass * remove useless file * remove bad test * tests for all proposers * parallelize tests * rearrange deps * still wip * remove warning on yarn * some cleanups to package * considerable improvements to code quality in machine * remove 4447 json network * checkpointing stuff * commitment tests passing * mostly tested machine functionality * better testing * make explicit unused files * nice script stuff * fix tests after ethers update * updates * clean up tests to handle immutable * code cleanup * add comment * remove files that should not be here * remove files that should not be here * remove yarn clean * remove machine dependency on cf.js * minor readme touch up * fix lint * remove bold and normal in build.sh * Update packages/machine/src/models/state-channel.ts Co-Authored-By: snario <[email protected]> * Update packages/machine/src/ethereum/install.ts Co-Authored-By: snario <[email protected]> * Update packages/machine/src/models/app-instance.ts Co-Authored-By: snario <[email protected]> * move utils to module import * typo * add doc * typo * replace console.log with tests * import from ethers/* * change dependencies to devDepencies for apps package * switch to resolver map * type arbitrary state objects better * use template literals for error messages * inline Map mutations for cleaner code * abridge if statement * Delegatecall -> DelegateCall * Update packages/machine/src/ethereum/utils/free-balance.ts Co-Authored-By: snario <[email protected]> * remove unused class * Update packages/machine/src/ethereum/uninstall.ts Co-Authored-By: snario <[email protected]> * Update packages/machine/src/ethereum/uninstall.ts Co-Authored-By: snario <[email protected]> * Update packages/machine/src/ethereum/install.ts Co-Authored-By: snario <[email protected]> * symlink .soliumrc.json * use ethers module imports some more * shorten tsconfig in @apps * add --detectOpenHandles back * shorten lines in .gitignore * re-sort .soliumrc.json file * uncomment critical line in StateChannelTransaction * Update packages/contracts/contracts/libs/LibStaticCall.sol Co-Authored-By: snario <[email protected]> * better documentation of appStates mapping * better documentation of appResolutions mapping * rename _id to id in all .sol files * better docs on setState method * remove test:windows because its the same * re-version @types to 0.0.1 * rename TIMEOUT variable to ONCHAIN_CHALLENGE_TIMEOUT in a test * import Wallet in constants.ts * remove some commented out code * fresh conflict-free yarn.lock file * rename monotonicSeqNo according to Xuanjis's proposed scheme 'counterfactual/monorepo#163 (comment)' * use ReadonlyMap not Readonly<Map * add ethereum-waffle typescript typings * remove ts-ignore * update ethers to 0.4.20 * remove unnecessary build step for @types * remove some more tsignores * remove unused imports * reintroduce script to build types * add apps to build string * better variable naming * fix @types build process * add ganache-cli to test process of machine * fix typo * undo export of cf * Add infrastructure to run ganache-cli tests in machine * Fix ordering of fields in tuple causing encoding to be incorrect * Fix incorrect block.number / finalizesAt value comparison * Add MinimumViableMultisig to the migrations (for use with proxyfactory) * Remove --reset flag on truffle migrate for contracts * Rename appState to encodedAppState on SetStateCommitment for clarity * Use encodePacked not encode for getTransactionHash on Multisig * Add encodedTerms method on AppInstance * Change default timeouts to 10 not 100 (for tests) * Update the types folder to correctly export types and enums * Update yarn.lock * Working blockchain test * rename file * Fix timeout test after value change * Switch arrow notation to function for encodeTransactions * Add missing variable usage in test.sh * more descriptive test description * Fix a linting error * add types to front of build.sh string * use relative importing in contracts as it turns out it is causing double compilation * fix an incorrect merge from earlier of master * nearly finished working merge of virtualAppAgreement test * Remove multisig.ts util file * testing rename of files to see how git treats the rename * Rename files to old names to fix git history * Add build symlink * ensure tests pass * fix some linting issues * update circleci config * dont delete .env when cleaning * imrpoved usage of exit signals for bash trap * ensure circleci runs tests correctly * update some README * Update README.md * Rename truffle.js to truffle-config.js * Replace ethers usage to always import from module (#364) * Replace all ethers utils with module imports * Fixed a few places I missed the first time * Update rollup for cf.js to include ethers.constants * Rename appCfAddress to appInstanceId (#372) * Add @firebase/app-types to devDependencies on node * fix some linting issues * Add apps back to build file * Update packages/machine/test/integration/setup-then-set-state.spec.ts Co-Authored-By: snario <[email protected]> * Revert syntax error from github suggestion * Remove duplicate code * yarn.lock * fix solium to 1.1.8 * update waffle
Introduction
This pull request includes several changes to the codebase, all spawned originally from the introduction of a new contract:
AppRegistry.sol
. This contract entirely replacesAppInstance.sol
. The motivation for this change was to remove the necessity to have to deploy a contract in the event of a protocol breakdown (i.e. a "dispute"). Instead, they simply submit a transaction to the global singletonAppRegistry
contract with some metadata pertaining to the particular AppInstance they are referring to.NOTE: I have written a small glossary at the bottom of this document to help articulate the differences between some core terms.
Core Concept Changes
Updated
machine
middleware signatureMiddle ware now takes three arguments:
message
: AProtocolMessage
objectcontext
: An object that persists through execution of a flow that stores "remembered" data from prior middleware execution including astateChannel
object, acommitment
object, and potentially asignatures
objectstateChannel
: The state channel that was initially passed into the execution of the middleware. The idea is that the execution of a protocol is intended to transform this StateChannel into a new version of a StateChannel representative of the updates the protocol made to the objectYou can see how
STATE_TRANSITION_PROPOSE
is implemented for the Setup Protocol for example:monorepo/packages/machine/src/protocol/setup.ts
Lines 62 to 69 in 578a7e2
cfaddress
no longer existsThe notion of a
cfaddress
is now gone. Replacing it is the unique AppInstanceId which is described in the glossary below. Incf.js
I did re-implement thecfAddress()
method to compute the AppInstanceId instead, but I did not change the public API.Introduction of AppRegistry
The actual
AppRegistry.sol
file is pretty simple because it uses a "mixin" architecture:monorepo/packages/contracts/contracts/AppRegistry.sol
Lines 12 to 22 in 578a7e2
Each mixin implements a public facing method of the
AppRegistry
contract.MixinAppRegistryCore
implements some helper external / accessor methods:getAppChallenge(bytes32 _id) returns (LibStateChannelApp.AppChallenge)
isStateFinalized(bytes32 _id) returns (bool)
getResolution(bytes32 _id) returns (Transfer.Transaction)
MixinSetState
implements thesetState
methodMixinSetStateWithAction
implements thesetStateWithAction
methodMixinSetResolution
implements thesetResolution
methodMixinCancelChallenge
implements thecancelChallenge
methodNaming Changes
CfOperations
I renamed CfOperations to "commitments". This is what they are as described in the specs. They are individual transactions which could be sent to the blockchain that have a
hashToSign()
. The folder these things now live in is under theethereum
folder in the machine package since these particular commitments are coupled to the Ethereum blockchain. Here is theInstallCommitment
for example and the specifications that you can cross-reference for implementation correctness.PeerBalance
This was a class that basically described a tuple of (a) "who" and (b) "how much" abstractly. It was re-used for install sub-deposits and uninstall resolution proposals. Hidden in its obscure description and name however was the fact that this thing hard-coded Wei balance changes to the ETH Free Balance of the underlying state channel. So, I've gotten rid of its usage and replaced it with arguments to the install protocol and uninstall protocol that are far more explicit about their usage:
aliceBalanceIncrement
andaliceBalanceDecrement
(and the Bob counterparts). You can see usage here.Pull Request Changes
I tried to order this in order of importance and relevance...
Introduction of
StateChannel
andAppInstance
modelsWe were desperately needing clearly defined immutable data structures that represent a state channel and an app instance. This is what these were added for. They replace
StateChannelInfoImpl
which was an attempt at this object and the sometimes used legacy.AppInstance.As alluded to above, these classes have been implemented to be immutable meaning all methods on the classes return a new instance of the class with some values changed. This makes the implementation details of the machine much easier to reason about in my opinion.
Note these classes do not use
immutable.js
yet although they would benefit from itRecommendation: Read the tests!
Removing the global
ganache-cli
I felt like we were unnecessarily coupling the test environments of the packages by requiring there to be a global
ganache-cli
blockchain running for all of the tests. Instead, I've separated out the execution of theganache-cli
blockchain to be local to each package. So, the contracts repo spins its own instance up, runs migrations, runs tests, spins it down. See here.The
apps
packageI moved out some contract code for apps like TicTacToe, Nim, etc into a new package called
apps
which just is a collection of random apps. For this repo I used a new tool that replaces Truffle for testing called Waffle. Using it made the tests way cleaner since it is based onethers
and they run really fast (like 1 second).I tried using Waffle on the main
contracts
repo too but hit some bugs. I have a branch off this PR that implements it. I'm waiting on Waffle to mature more for that PR but for theapps
repo the tests are basic enough that we don't encounter any issues.Significantly better testing
Instead of constructing CfOperations / Commitments and then checking if the
hashToSign()
method returned a string that was equal to the manually constructed version of it in a test file, now we construct the actual commitment (the transaction) and then decode the calldata piece by piece and check that it is equal to the expected function call to a contract. It is easy to now read a file like the install commitment test file and cross-reference it with the specifications of the install protocol to verify its correctness.Code Cleanliness
Remove all use of
legacy
frommachine
I was getting really frustrated with this bizarre dependency so I removed every use of anything to do with
cf.js/legacy
methods and defined my own replacements to all of them.The
@counterfactual/types
packageI added a new package to store some types I constantly was reusing. I also added a file in the machine called
protocol-types-tbd
. Both of these probably ought to be merged in some way with thecommon-types
package that @mvanderh added.Stop manually specifying test files in
yarn test
for contractsAs @Alonski found out, it is frustrating to have to specify the names of the files you are testing. As you can see here I have removed this
package.json
config entirely and now everything "just works" because thetsc
build process actually places the built files inside thetest/
directory instead of creating a newdist/
subdirectory. This is where thetruffle test
expects the files to be. There is no downside to this as far as I can tell.Stop relying on Truffle migrations
I have the opinion that it is bad practice for the test files of the contracts repo to rely on the migrations being run for them to work (e.g, using
.deployed()
method onTruffleContract
) and that, instead, we should deploy and manually link every contract inline in the test. This de-couples the tests from Truffle's migrations framework which I think is good practice.Proper use of
type
,interface
, andclass
We had been using
class
to define too many kinds of things which has no business being classes. Here is a classic example of that. Instead I followed this rule:type
when defining the structure of some kind of data structure that will be passed aroundinterface
when defining the structure of an object that is required on a public API methodclass
when there is a requirement for the thing to be uniquely identifiable and persistentUse
ethereum-waffle
forchai
pluginsWe used to use three separate plugins
chai-bignumber
,chai-as-promised
,chai-string
or something like that in order to add some brevity to our test files with custom matchers like.to.be.bignumber.eq
. It turns out theethereum-waffle
project implemented many of the same things and exported their own chai matcher which we now solely re-use.Remove
AbstractContract
Since we no longer really ever need to do any linking any place except test code (and even then it is entirely optional because Truffle could do it by itself as described above) the
AbstractContract
class and its associated utilities were mostly just complicating the code in a way that I felt was unhelpful.Importing ethers utils and constants as modules
Pretty much everywhere I've stopped importing
ethers
itself and instead import the utility methods and constants individually. For example,I think it really helped clean the code up.
Replace
rm -rf <hard coded folders>
withgit clean -Xdf
For the
yarn clean
command we relied on manually selecting folders to delete when executing the command, but I think usinggit
s built inclean
command achieves the same thing automatically.Glossary
AppInterface
The equivalent of a contract "ABI" for an off-chain application on Counterfactual.
Properties
addr
: The address of an on-chain contract where the contract with the following interface is deployed.applyAction
,resolve
,isStateTerminal
,getTurnTaker
: Fourbytes4
sighashes which are used by theAppRegistry
to makestaticcall
functions to the address of the contract. It is very important to understand that this is solely needed because the contracts use a "hack" to automatically decode ABIEncoderV2 encoded structs asbytes
data types in the calldata of function calls. With Solidity 0.5, these fields are no longer necessary because of the introduction ofabi.decode
.stateEncoding
: A string representation of the data type that represents the state. For example,tuple(address foo, bytes32 bar)
.actionEncoding
: A string representation of the data types that represents an action on the state. This can beundefined
for applications that do not define anapplyAction
method.Terms
An object that encodes metadata regarding the amount of blockchain state (e.g., funds) that an application has control over in the context of the Counterfactual framework.
Properties
assetType
: An enum representing some well-established asset types e.g., ETH, ERC20limit
: An integer representing some notion of the amount of that asset class being controlled by this application.token
: If ERC20, the address of the token on-chain that represents the token.AppIdentity
An object which represents a unique pointer to an off-chain application in a particular state channel.
Properties
owner
/multisigAddress
: The address of an on-chain contract for which, if a transaction is submitted to the blockchain to update the state of this AppInstance wheremsg.sender == owner
then it is considered an "authorized" transaction.signingKeys
: An array of addresses for which, ifmsg.sender
is not theowner
, then any transaction submitted to the blockchain that updates the state of the AppInstance must have also asignatures
argument which includes a copy of a signature from every key insigningKeys
to be considered "authorized".appInterfaceHash
: The hash of an AppInterface object.terms
: The hash of a Terms object.defaultTimeout
: The number of blocks by default (it can be overrided) that theAppRegistry
will wait for newer state in the case where a "dispute" is triggered.id()
: A computed property, also known as "AppInstanceId" that is computed askeccak256(abi.encodePacked(owner, signingKeys, appInterfaceHash, terms, defaultTimeout))
.AppInstance
An instance of an off-chain application in a particular state channel.
Properties
latestState
: The latest state of the AppInstance in the channel e.g.,{ foo: 0x, bar: 0x }
latestNonce
: The latest nonce / version number of this statelatestTimeout
: The latest agreed upon timeout for which this state would have on-chain (defaults toidentity.defaultTimeout
.appSeqNo
: The sequence number / global unique nonce inside the channel for this AppInstanceTODO: Not implemented yet:
rootNonceDependency
: The value of the root nonce dependency for this appuninstallKey()
: The computed "key" that would be used for theNonceRegistry
that a conditional transaction would rely on not having been set to "UNINSTALLED" (i.e.1
). Computed askeccak256(appSeqNo)
.identity()
: An AppIdentity object, as described above.id()
: The computedid()
method of the identity above.StateChannel
An instance of a state channel.
Properties
multisigAddress
: The on-chain address of a single multisignature wallet that stores the funds.multisigOwners
: The addresses of the public keys which are the owners of the multisig wallet.appInstances
: A mapping of AppInstance.ids to AppInstances that are currently activefreeBalanceIndexes
: A mapping from AssetType to the unique AppInstance.id inappInstances
TODO: Need a better name:
monotonicallyIncreasingSeqNo
: A monotonically increasing number representing the number of apps ever installed all-time.TODO: Not implemented yet:
rootNonceNumber
: The value of the root nonce at the present