Skip to content
This repository has been archived by the owner on Dec 13, 2019. It is now read-only.

Remove MultiSend, move activeApps to FreeBalanceState, upgrade protocols, and more #1774

Merged
merged 3 commits into from
Jul 9, 2019

Conversation

ldct
Copy link
Member

@ldct ldct commented Jun 28, 2019

This pull request contains several changes rolled into one. They are:

Major

Removing MultiSend

Previously we used the MultiSend contract for the purposes of atomically updating the free balance state and signing a multisignature wallet delegatecall transaction. We needed this atomicity because we had a design goal of one-round-trip per protocol execution. We've relaxed this design goal and are okay with the protocols requiring multiple round trips, now.

Add activeApps to the Free Balance state

Instead of using uninstallKey parameters on the pre-signed transaction made to an AppInstance to validate if it is in fact a funded App or not, we instead rely on a master list of "active" (a.k.a. funded) apps. This is an array on the state of the free balance. You can see how this logic plays out here:

bytes32[] memory activeApps = abi.decode(
appRegistry.getOutcome(freeBalanceAppIdentityHash),
(FreeBalanceAppState)
).activeApps;
bool appIsFunded = false;
for (uint256 i = 0; i < activeApps.length; i++) {
if (activeApps[i] == appIdentityHash) {
appIsFunded = true;
}
}
require(appIsFunded, "Referenced AppInstance is not funded");

This means the UninstallKeyRegistry is also removed.

Updating protocols to manage effected changes

The following protocols have been upgraded:

  • Install
  • InstallVirtual
  • Uninstall
  • UninstallVirtual

In the case of the installation protocols, they now have an extra round trip per user involved. This is because to make an installation secure you need to first see the unanimously signed conditional transaction (the call to the ConditionalTransactionDelegateTarget) and then you feel OK with signing the update to the Free Balance (both activeApps and the relevant balance updates).

In the case of the uninstall, instead of setting a bit on the UninstallKeyRegistry now we set the state of the Free Balance object and remove the app from the activeApps list.

  • Still need to update the Mermaid diagrams!
  • Need to deploy new contract addresses

Minor

Extend FreeBalanceState

export type FreeBalanceState = {
balancesIndexedByToken: {
[tokenAddress: string]: PartyBalance[];
};
activeAppsBloomFilter: { [appInstanceIdentityHash: string]: true };
};

Now it includes balancesIndexedByToken and activeAppsBloomFilter whereas previously it was just this:

export type FreeBalanceState = {
  [tokenAddress: string]: PartyBalance[];
}

Introduce ConditionalTransaction

This is a new commitment type which represents a multisignature wallet delegatecall function call to the ConditionalTransactionDelegateTarget. Previously this was encoded inside the "install commitment". See here:

Capture d’écran 2019-07-07 à 18 32 15

Remove TwoPartyVirtualEthAsLump.sol

We had a special case contract that handled virtual app agreement interpretation in a hard-coded way for TwoPartyFixedOutcome outcome types. Instead, now we re-use ConditionalTransactionDelegateTarget as we do for all AppInstances but the clientside code is now hard-coded to the TwoPartyFixedOutcomeETHInterpreter for now instead.

Introduce IdentityApp

For test scenarios this is a nice app. It is an app whose computeOutcome return value is its encodedState.

Rename TwoPartyEthAsLump to TwoPartyFixedOutcomeETHInterpreter

This name was also confusing and so now it references the exact outcome type and asset class it is meant to be an interpreter for. Additionally, the virtual equivalent TwoPartyVirtualEthAsLump which manages the distribution of assets between an endpoint and an intermediary was renamed to TwoPartyFixedOutcomeFromVirtualAppETHInterpreter.

Rename ETHBucket to FreeBalanceApp

Since the Free Balance is taking on a larger role, I decided to give it a more unique name.

Removing rollup from @counterfactual/chain

The only consumers of this package were internal to the codebase and didn't require anything other than access to the TypeScript source code, so I removed the rollup dependency and build step.

Miscellaneous Code Cleanup

  • Renamed latestversionNumber to latestVersionNumber
  • Rename several function names to be more clear
  • Make several methods in free-balance.ts functional instead of imperative
  • Add lots of documentation in places
  • Rename validateSignature to requireValidSignatureOrThrowError

@ldct ldct changed the title [contracts] make free balance not uninstallable [contracts] use free balance app state to store list of active apps Jul 1, 2019
@Alonski
Copy link
Member

Alonski commented Jul 5, 2019

Maybe we should wait to merge in Joels fixes before merging this, or anything else, in?

@snario
Copy link
Contributor

snario commented Jul 5, 2019

I'm going to split it up into multiple smaller PRs I think, consider this a draft atm

@snario snario changed the title [contracts] use free balance app state to store list of active apps [contracts] Remove MultiSend, move activeApps to FreeBalanceState, and more Jul 7, 2019
@snario snario changed the title [contracts] Remove MultiSend, move activeApps to FreeBalanceState, and more Remove MultiSend, move activeApps to FreeBalanceState, and more Jul 7, 2019
@snario snario changed the title Remove MultiSend, move activeApps to FreeBalanceState, and more Remove MultiSend, move activeApps to FreeBalanceState, upgrade protocols, and more Jul 7, 2019
@snario snario self-assigned this Jul 7, 2019
@snario snario added 💎 Ethereum 📦 Contracts code related to packages/contracts 📦 Node code related to packages/node 🔐 Protocol Related labels Jul 7, 2019
packages/node/src/models/free-balance.ts Outdated Show resolved Hide resolved
packages/apps/migrations/2_deploy_contracts.js Outdated Show resolved Hide resolved
packages/apps/test/nim.spec.ts Outdated Show resolved Hide resolved
packages/chain/package.json Outdated Show resolved Hide resolved
import { NetworkContext } from "@counterfactual/types";
import { ContractFactory, Wallet } from "ethers";

export async function configureNetworkContext(wallet: Wallet) {
export type NetworkContextForTestSuite = NetworkContext & {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

separate PR

packages/contracts/test/challenge-registry.spec.ts Outdated Show resolved Hide resolved
Copy link
Member

@cf19drofxots cf19drofxots left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides separating into different PRs, lgtm

@@ -55,7 +58,7 @@ beforeAll(async () => {
/**
* @summary Set up a StateChannel and then install a new AppInstance into it.
*
* @description We re-use the ETHBucket App (which is the app ETH Free Balance uses)
* @description We re-use the FreeBalanceApp App (which is the app ETH Free Balance uses)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is ETH Free Balance in this case?

defaultAbiCoder.encode(
["uint256"],
[freeBalanceETH.coinTransferInterpreterParams!.limit]
["tuple(uint256 limit)"],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This encoding should be defined in one place and used throughout the rest of the codebase.

@ldct ldct force-pushed the ldct/active-bucket branch 2 times, most recently from 57d3870 to d297618 Compare July 8, 2019 22:24
@snario snario merged commit 05b9989 into master Jul 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
📦 Contracts code related to packages/contracts 💎 Ethereum 📦 Node code related to packages/node 🔐 Protocol Related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants