[contracts] Make CounterfactualApp an abstract contract

[snario]

It should be possible to create contract X is CounterfactualApp {} and have that exist as a real app inside the framework.

[adklempner]

Is this issue still relevant? Looks like CounterfactualApp is already an interface, and all the contracts in packages/apps use the * is CounterfactualApp pattern

[cf19drofxots]

I don't think so, but I'll let @snario or @IIIIllllIIIIllllIIIIllllIIIIllllIIIIll confirm

[adklempner]

I found that the pattern isn't implemented in one of the default-apps, ETHBalanceRefundApp. It can't be an instance of CounterfactualApp because its implementation of resolve reads from state:

contracts/default-apps/ETHBalanceRefundApp.sol:24:18: TypeError: Function declared as pure, but this expression (potentially) reads from the environment or state and thus requires "view".
    amounts[0] = address(state.multisig).balance - state.threshold;
                 ^-----------------------------^

[alxiong]

@adklempner RefundApp is not meant to be a CounterfactualApp, it's used during deposit protocol, to ensure newly deposited eth/token into the multisig can be withdrawn. The documentation is still under its way to specify this.

But yes, you could argue that we should remove the import "../CounterfactualApp.sol imo.

[snario]

It's actually ambiguous whether or not RefundApp should be a CounterfactualApp. I mean, it is a necessary default app that the Node relies on. I think it would be useful to keep them as CounterfactualApps to be honest.

Also in the original statement of this issue I specifically put that this code:

contract X is CounterfactualApp {}

Should be a valid app in the framework. Meaning you could install X (despite the fact it does nothing and is useless).

Maybe abstract contract was the wrong word choice; I think I meant just "contract"; as in applyAction, resolve, isStateTerminal, and getTurnTaker have default implementations. Default applyAction reverts with an error, resolve would return a null transaction, getTurnTaker would return the 0th index signingKey perhaps, and isStateTerminal would return false.

This is also debatable a bad idea; maybe it shouldn't be possible to inherit defaults here.

[alxiong]

Ah, I see. I do agree with converting ETHRefundApp to be CounterfactualApp, but prefer the current "abstract + implementation" over "default implementation + overload function".

To convert,

1. AppState should be changed, (e.g. threshold --> amount, multisig.balance-threshold --> amount directly)
2. add "zombie" function for 3 other functions (i.e. revert("not implemented");
3. the first point will break integration tests in Node, so there's respective fix there.
4. redeploy contracts to testnets.

[ldct]

why shouldn't the appState include threshold?

[alxiong]

because to comply with pure accessibility, reading the balance needs to have a workaround, so I assume that just specifying the exact withdrawable amount would also work.

[snario]

Fundamentally the refund app will need to be a view function because it is reading the chain to confirm that a deposit occurred. This breaks the pure pattern in this one case.

I think the general rule of thumb is that if you break the pure patten then you are in highly risky territory so the Node software needs to be aware of that and have special handlers for these cases. For example, reading chain state now comes with it the problem that getting data from blockchains is tricky. For example, how many confirmations do you wait? What provider do you use; Infura, DappNode, run your own? What if the provider is load-balanced without syncronized state? etc...