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

[monorepo] replace Transfer.sol mechanism with Interpreters #1263

Merged
merged 14 commits into from May 13, 2019

Conversation

Projects
None yet
4 participants
@IIIIllllIIIIllllIIIIllllIIIIllllIIIIll
Copy link
Member

commented Apr 11, 2019

This PR refactors the contracts and node and fixes: #1222, #1157, #1442

The design is as follows. App Definition resolve functions now return unstructured data which is to be consumed by an "interpreter" contract which runs in the multisig context and handles state deposit payouts in dispute scenarios. App Definitions are classified by "resolve type", e.g.,

  • TwoPartyOutcome: the app is some kind of 2-party game, and the resolve function returns a uint256 in the set {0, 1, 2} which means "player 1 won, player 2 won, draw" respectively
  • ETHTransfer: the resolve functions returns a list of (to, value) tuples

State deposits constrain what apps they can be used with: for instance, a deposit of ETH can be used for apps of resolve type ETHTransfer or TwoPartyOutcome, but a deposit of two ERC721 NFTs can only be used for TwoPartyOutcome.

hard-coded assumptions

  • virtual apps always resolve to type 2-party outcome

temporary hacks to be removed in the future

  • A resolveType function is introduced in all app definitions. This is used by the uninstall protocol to determine how to update the free balance during uninstall.
  • A limitOrTotal field is added to the AppInstance class. For app instances interpreted by ETHInterpreter, this represents a limit on the total ETH; for those interpreted by TwoPartyEthAsLump, this represents the total funds committed to the app.
  • The ttt bot assumes that it is player 2
  • interpreterHash is computed but not checked in StateChannelTransaction

@IIIIllllIIIIllllIIIIllllIIIIllllIIIIll IIIIllllIIIIllllIIIIllllIIIIllllIIIIll force-pushed the ldct/transfer branch 3 times, most recently from 13e793d to 7129182 Apr 11, 2019

Show resolved Hide resolved packages/apps/waffle.js Outdated
Show resolved Hide resolved packages/contracts/waffle.js Outdated
@AlexXiong97

This comment has been minimized.

Copy link
Collaborator

commented Apr 13, 2019

@IIIIllllIIIIllllIIIIllllIIIIllllIIIIll two extra comments:

  1. better comments would help. (e.g. I don't really know what am I supposed to feed into resolution and interpreterParams
  2. in the future, it would be more friendly if we provide a predefined set of interpreter options (i.e. preset) for most use case such that developers can just choose one (from an enum, which contains a list of common interpreters) by specifying a number, or they can customize their own for more complicated app.

in general, I do think current change is great and will support coinshuffle app requirement. Great work!

@IIIIllllIIIIllllIIIIllllIIIIllllIIIIll IIIIllllIIIIllllIIIIllllIIIIllllIIIIll force-pushed the ldct/transfer branch 7 times, most recently from 894e000 to 59245be Apr 15, 2019

@IIIIllllIIIIllllIIIIllllIIIIllllIIIIll

This comment has been minimized.

Copy link
Member Author

commented Apr 22, 2019

might be blocked on duaraghav8/Ethlint#261

@IIIIllllIIIIllllIIIIllllIIIIllllIIIIll IIIIllllIIIIllllIIIIllllIIIIllllIIIIll force-pushed the ldct/transfer branch 5 times, most recently from 23cb32c to f6cef5e Apr 22, 2019

Show resolved Hide resolved tsconfig.json Outdated

@IIIIllllIIIIllllIIIIllllIIIIllllIIIIll IIIIllllIIIIllllIIIIllllIIIIllllIIIIll force-pushed the ldct/transfer branch 2 times, most recently from f8fcb35 to b8bc4d2 May 10, 2019

IIIIllllIIIIllllIIIIllllIIIIllllIIIIll added some commits May 13, 2019

fix
@snario

snario approved these changes May 13, 2019

@IIIIllllIIIIllllIIIIllllIIIIllllIIIIll IIIIllllIIIIllllIIIIllllIIIIllllIIIIll changed the title [contract] Interpreters [monorepo] replace Transfer.sol mechanism with Interpreters May 13, 2019

@IIIIllllIIIIllllIIIIllllIIIIllllIIIIll IIIIllllIIIIllllIIIIllllIIIIllllIIIIll merged commit 3f71398 into master May 13, 2019

16 of 24 checks passed

Header rules - high-roller-staging No header rules processed
Details
Header rules - playground-production No header rules processed
Details
Header rules - playground-staging No header rules processed
Details
Header rules - tic-tac-toe-staging No header rules processed
Details
Pages changed - high-roller-staging 2 new files uploaded
Details
Pages changed - playground-production 5 new files uploaded
Details
Pages changed - playground-staging All files already uploaded
Details
Pages changed - tic-tac-toe-staging All files already uploaded
Details
Mixed content - high-roller-staging No mixed content detected
Details
Mixed content - playground-production No mixed content detected
Details
Mixed content - playground-staging No mixed content detected
Details
Mixed content - tic-tac-toe-staging No mixed content detected
Details
Redirect rules - high-roller-staging 1 redirect rule processed
Details
Redirect rules - playground-production 1 redirect rule processed
Details
Redirect rules - playground-staging 1 redirect rule processed
Details
Redirect rules - tic-tac-toe-staging 1 redirect rule processed
Details
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: run-non-playground-tests Your tests passed on CircleCI!
Details
ci/circleci: run-playground-tests Your tests passed on CircleCI!
Details
ci/circleci: run-tslint Your tests passed on CircleCI!
Details
netlify/high-roller-staging/deploy-preview Deploy preview ready!
Details
netlify/playground-production/deploy-preview Deploy preview ready!
Details
netlify/playground-staging/deploy-preview Deploy preview ready!
Details
netlify/tic-tac-toe-staging/deploy-preview Deploy preview ready!
Details

@snario snario deleted the ldct/transfer branch May 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.