swap: use the SimpleSwapFactory for deployment and verification

[ralph-pichler]

This PR

• uses the new SimpleSwapFactory to deploy and verify contracts
• uses a newer version of SimpleSwap (same API but different bytecode due to newer compiler)
• looks up the factory contract address for common network ids (here only ropsten (3))
• adds a swap-chequebook-factory flag to override this
• lets node startup fail if swap is enabled and no valid factory is found or supplied
• deploys a factory during testing (in init())

The naming of things is a bit inconsistent. In contract I have a SimpleSwapFactory because that's how the contract is called but in swap it is referred as chequebookFactory because everything is called chequebook there. Any suggestions on how to make this better are welcome.

Starting with this PR contract abstractions now maintain their own reference to their backend. SimpleSwapFactory was already written this way and Contract was refactored. This is because a contract abstraction is always tied to a backend at creation anyway and calling anything related to it (e.g. WaitFunc) with a different backend is always wrong.

See https://hackmd.io/yZLFmgdSRDCMEpBXCiBeBA?view for how to deploy a factory for testing purposes.

Note that while this PR addresses the issue of maliciously deployed chequebook contracts it actually introduces another security issue. Since the factory stores the addresses of deployed chequebooks that too could be manipulated during deployment of the factory. This is a lesser issue though as factories are deployed by Swarm Developers and their address is hardcoded into the client for common networks. If somebody passes the swap-chequebook-factory it is their responsibility that the factory was initialised properly. There are several ways this could be addressed if needed but I would leave that to a future PR.

Since most tests now require a SimulatedBackend to work, one is automatically created if nil is passed in. The value submitted to chequebooks at test deployment has also been adjusted to avoid bouncing cheques.

closes #1809
closes #1641
closes #1583

[ralph-pichler]

@acud sorry, mis-clicked and requested your review by accident.

[santicomp2014]

Great job Ralph, the implementation looks good, I'm not really an expert in the Contract side, but I do believe this is a good way to prevent abuse of the system thru a factory pattern.

There are only two things I see with this PR is the naming, But the naming will be addressed in other PRs I assume.
The other point is what @Eknir commented.

Proposing to have the factory is a separate directory, as now the functions of factory.go are in the same namespace as swap.go, which is problematic because there are functions which overlap, such as instanceAt and factoryAt (which do essentially the same, so it would be better to name them the same as well).

[holisticode]

We don't know concurrency control here right, because this is only used at startup right?

[ralph-pichler]

Yes, this only runs once during the constructor of Swap.

[holisticode]

So how do you get to exactly this number and why is it a good value?

[ralph-pichler]

It's the real deployment cost (which is constant and can be determined by deploying through truffle) rounded up to the next 100000. I'll clarify this in a comment.

[holisticode]

Is this ok to wait here until WaitFunc terminates? Couldn't that potentially be long? Wouldn't this block the entire boot process?

[ralph-pichler]

Yes, but that's how it was previously too (except there we were waiting for deployment of the chequebook directly).

[holisticode]

We could just

if !isDeployed {
		return ErrNotDeployedByFactory
	}
	return err
}

[ralph-pichler]

The problem with this approach is that if err is not nil then isDeployed will also be false. So for actual errors it would also return ErrNotDeployedByFactory instead of err.

[holisticode]

Does it still make sense here to actually do deployRetries here anymore? Isn't it the case that if it failed, we just return anyway and don't iterate anymore?

[ralph-pichler]

Hm, I don't think i did that intentionally. I changed it back to continue.

That being said I'm not sure if the deploy loop is useful. What kind of error are we trying to handle here? The only situation I can think of where it would not work and then work a little latter was if the network connection to the eth node got cut (in that case it's unlikely it will work again after 1s too).

[Eknir]

Agreed about the deploy loop being redundant, I would propose to remove this, or add an issue so we won't forget to do this at a later point.

[Eknir]

Thanks for addressing the comments.
There are still open issues with regards to the naming, but these can be addressed in a subsequent PR. Good work! Approved.

[janos]

Tests in swap package are consuming a large amount of memory according to mem profile
profile001.svg.zip (GitHub does not support svg attachments, hence the zip), and discovered on 32-bit system https://ci.appveyor.com/project/ethersphere/swarm/builds/28196921/job/5yls2dhjvq13pn9p#L945.

The problem is in SimulatedBacked cache which is not released on Close. I know that this was addressed earlier at go-ethereum and that it should be fixed, but according to this, it appeared again. A similar workaround like initial one can be done (minimal set of changes):

diff --git a/swap/swap_test.go b/swap/swap_test.go
index 103907f9e..276b1663c 100644
--- a/swap/swap_test.go
+++ b/swap/swap_test.go
@@ -83,20 +83,32 @@ type swapTestBackend struct {
        cashDone chan struct{}
 }
 
+func (b *swapTestBackend) Close() error {
+       // Do not close SimulatedBackend as it is a global instance.
+       return nil
+}
+
+func TestMain(m *testing.M) {
+       exitCode := m.Run()
+       // Close the global default backend
+       // when tests are done.
+       defaultBackend.Close()
+       os.Exit(exitCode)
+}
+
 func init() {
        testutil.Init()
        mrand.Seed(time.Now().UnixNano())
        swapLog = log.Root()
 }
 
+var defaultBackend = backends.NewSimulatedBackend(core.GenesisAlloc{
+       ownerAddress:       {Balance: big.NewInt(1000000000000000000)},
+       beneficiaryAddress: {Balance: big.NewInt(1000000000000000000)},
+}, 8000000)
+
 // newTestBackend creates a new test backend instance
 func newTestBackend() *swapTestBackend {
-       gasLimit := uint64(8000000)
-       defaultBackend := backends.NewSimulatedBackend(core.GenesisAlloc{
-               ownerAddress:       {Balance: big.NewInt(1000000000000000000)},
-               beneficiaryAddress: {Balance: big.NewInt(1000000000000000000)},
-       }, gasLimit)
-
        // commit the initial "pre-mined" accounts (issuer and beneficiary addresses)
        defaultBackend.Commit()

to have only one instance of the backend, which can be a temporary solution as the memory is still not released and it is questionable if it is correct to have a global backend for all tests.

With this change the memory profile looks like this
profile002.svg.zip.

[mortelli]

thanks for taking on this task @ralph-pichler 👏

no blockers, but some questions, comments & suggestions below.

feel free to merge after addressing any number of them