swarm, swap: pass chequebook address at start-up #1718

Eknir · 2019-09-02T07:55:25Z

This PR aims to complete 1548 and the duplicate (?) 1599.

No unit tests were provided, as this functionality resides largely in swarm.Start. Creating an integration test was attempted, but this is not possible with a simulated backend.

To test manually (I used Rinkeby):

set swap.AllowedNetworkID to 4
ask me for some test ethers (if you don't have them) on the account which you use to start Swarm
start Swarm without provided chequebook:
swarm --swap --swap-backend-url https://rinkeby.infura.io/v3/<INFURA_KEY>
start Swarm with provided chequebook: swarm --swap --swap-backend-url https://rinkeby.infura.io/v3/<INFURA_KEY> --chequebook <Check the log from step 3>

An Infura key can be created by logging in at infura.io. I can give mine as well in PM.

With regards to 1599: it seems to me that it is suggested here that we have to store the chequebook to disk, such that it can be automatically re-used the next time a node boots up. I would propose to create a separate issue for this.

…ss on swap.new, create function NewInstanceAt

vojtechsimetka

Please consider my approval only as an information I saw it and it fulfils the issue description. I tried to run it the code looks good but I am no go expert :)

ralph-pichler · 2019-09-03T11:14:47Z

swap/swap.go

@@ -542,7 +546,18 @@ func (s *Swap) getContractOwner(ctx context.Context, address common.Address) (co
 	return contr.Issuer(nil)
 }

-// Deploy deploys the Swap contract
+// NewInstanceAt creates a new instance of the chequebook contract at address and sets chequebookAddr


I find this comment to be confusing. It doesn't really create a new instance of the contract but points to an existing one.

isn't it an instance on the go side? i.e. a new struct?

I would rename this to BindToContractAt or something similar. It is indeed very confusing, as it is a method on Swap, and it is not creating a new instance of Swap, and, as @ralph-pichler says, not even a new contract.

The comment should be changed accordingly

I agree about this being confusing.
I propose to change it to:

NewInstanceAt creates a new instance of an already existing chequebook contract at address and sets chequebookAddr

What do you think?

@mortelli , it is indeed only a new instance on the go-side.
To be precise:

type simpleContract struct { instance *contract.SimpleSwap }

, which is implementing the Contract interface

type Contract interface { // SubmitChequeBeneficiary submits a cheque to a given beneficiary SubmitChequeBeneficiary(opts *bind.TransactOpts, backend Backend, serial *big.Int, amount *big.Int, timeout *big.Int, ownerSig []byte) (*types.Receipt, error) // CashChequeBeneficiary cashes the cheque by the beneficiary CashChequeBeneficiary(auth *bind.TransactOpts, backend Backend, beneficiary common.Address, requestPayout *big.Int) (*types.Receipt, error) // ContractParams returns contract info (e.g. deployed address) ContractParams() *Params // Issuer returns the contract owner from the blockchain Issuer(opts *bind.CallOpts) (common.Address, error) // Cheques returns the last cheque for the given address Cheques(opts *bind.CallOpts, addr common.Address) (*ChequeResult, error) }

(see file contracts/swap.go).

since the instance exists in the blockchain but not on the go side, maybe use reference instead of instance; it seems talking about instances is a half-truth.

@ralph-pichler any suggestions for a better comment? what do you think of @Eknir's proposal?

I would definitely not use New. New in go usually is used to create a new instance of the caller, not a new instance of some other object.

Maybe BindToReferenceAt or BindToContractAt, ContractAt but not New...

Changed the function to ContractAt and updated the documentation to:

//ContractAt binds to an instance of an already existing chequebook contract at address and sets chequebookAddr

Final resolution: as this call is deferring to contract.InstanceAt, I see no reason why we would name it anything other than that.
Hence, I decided to change it to:

// InstanceAt creates a new instance of an already existing chequebook contract at address and sets chequebookAddr func (s *Swap) InstanceAt(address common.Address, backend swap.Backend) (err error) { s.contract, err = contract.InstanceAt(address, backend) if err != nil { return err } s.setChequebookAddr(address) return nil }

@holisticode , tagging you, as you already gave a thumbs-up on my previous proposed solution to make it ContractAt

swarm.go

swap/swap.go

swarm.go

holisticode

Very important PR, adds a pretty important functionality, thanks!

Please go through all the comments, mainly minor stuff about naming and go style

swarm.go

holisticode · 2019-09-03T15:04:31Z

swap/swap.go

@@ -542,7 +546,18 @@ func (s *Swap) getContractOwner(ctx context.Context, address common.Address) (co
 	return contr.Issuer(nil)
 }

-// Deploy deploys the Swap contract
+// NewInstanceAt creates a new instance of the chequebook contract at address and sets chequebookAddr


I would rename this to BindToContractAt or something similar. It is indeed very confusing, as it is a method on Swap, and it is not creating a new instance of Swap, and, as @ralph-pichler says, not even a new contract.

The comment should be changed accordingly

swap/swap.go

swarm.go

mortelli · 2019-09-03T15:41:43Z

swarm.go

-			return fmt.Errorf("Unable to deploy swap contract: %v", err)
+		if s.config.Contract != (common.Address{}) {
+			address := s.config.Contract
+			if err := cswap.ValidateCode(context.Background(), s.backend, address); err != nil {


shouldn't we call verifyContract instead of ValidateCode directly?

Nice one. Indeed that would be better!

With hindsight, I would actually propose to not change it to ValidateCode. The reason is that verifyContract is used as part of protocol.go to verify the chequebook contract of the counterparty. @ralph-pichler is doing research on how to make this check more elaborate.
In the instance where I am using the check, however, we actually only have to verify the deployedBytecode, so I would prefer to use the lower level call (cswap.validateCode) over the higher-level call (swap.verifyContract) which might add more checks in the future.

while i see your point, i think changes like these should be made in an atomic manner—i'm not a fan of leaving code around due to "just in case"- or "for later use"-type of situations.

right now, all verifyContract does is call ValidateCode. even their comments are very similar:

// verifyContract checks if the bytecode found at address matches the expected bytecode

// ValidateCode checks that the on-chain code at address matches the expected swap // contract code.

i would either:

remove ValidateCode and always call verifyContract directly

always call ValidateCode for consistency's sake

i will not consider this a blocker for approval, though 👍

Choose for option 2.
Meant I also had to rewrite some tests:
TestVerifyContract => TestValidateCode
TestVerifyWrongContract => TestValideWrongCode
I don't find this particularly nice, as the unit tests in swap are now actually testing functionality of contracts/swap. If we would like to change this, however, there is a need to create a new testSuite in contracts/swap.

I refrain from doing this at the moment but will put it on the agenda for the next sync meeting to hear to the opinion of team members.

thank you for putting this down for discussion.

i'm all for a new test suite if the situation calls for it 👍

swap/swap.go

mortelli

in broad terms this PR looks good to me 👍

i have made a few minor comments/corrections and left a couple of questions. i am happy to approve once these are resolved.

besides this, please:

rebase/merge this with master.
update the name of the PR to reflect the affected packages.

finally, a disclaimer: i have not run the manual tests myself.

santicomp2014 · 2019-09-04T20:01:30Z

swap/swap.go

+	return nil
+}
+
+// Deploy deploys the Swap contract and sets the contract address
 func (s *Swap) Deploy(ctx context.Context, backend swap.Backend, path string) error {


I'm confused as to what the path parameter does.
Is this to satisfy a Interface?, or is it something that is not being used.

And if this is the case what would be the use of such path.

I am confused about this as well. Actually removed it before when I was doing refactoring, but when I reverted the refactoring, I also left this one in now.
Tagging @holisticode , maybe he knows. I would personally remove it, as it does not seem to have a purpose

if the tests pass after removing this parameter, i say let's go ahead with this change

zelig

minor: please extract chequebook contract setup into the swap package to keep swarm.go setup lean.

zelig · 2019-09-05T04:54:48Z

swarm.go

-		err := s.swap.Deploy(context.Background(), s.backend, s.config.Path)
-		if err != nil {
-			return fmt.Errorf("Unable to deploy swap contract: %v", err)
+		if s.config.Contract != (common.Address{}) {


I think it would be nicer to extract this into a setup function. too much swap specific logic is here and makes swarm.go harder to read.

then you can also eliminate the else-s as you just return

mortelli · 2019-09-06T13:56:17Z

swap/swap.go

+	}
+	s.contract, err = contract.InstanceAt(address, s.backend)
+	if err != nil {
+		return err


since err is implicitly initialized, you can just return here

get rid of named paramater.
@mortelli our code policy is to write return arguments explicitly even if we have named return params

I would like to keep the initialization of err in the function initialization, because otherwise, I can't do

s.contract, err = contract.InstanceAt(address, s.backend)

Obviously, if it is a strict policy not to have named return parameters, I can do that, but in this case, I need 2 extra lines of code (to initialize a variable for the contractInstance, and assign this to s.contract). This was how I did it initially, but @holisticode asked me to do it in a one-liner.

this is fine. I also like sometimes that vars are initialised,
the policy is more about not having bare return-s if the function has return values.
it makes things more readable

mortelli · 2019-09-06T13:56:23Z

swap/swap.go

+		return err
+	}
+	s.setChequebookAddr(address)
+	return nil


I won't change this, due to Swarm code policy. Please tell me if you disagree.

mortelli · 2019-09-06T13:56:39Z

swap/swap.go

+func (s *Swap) StartChequebook(chequebookAddr common.Address) (err error) {
+	if chequebookAddr != (common.Address{}) {
+		if err := s.BindToContractAt(chequebookAddr); err != nil {
+			return err


since err is implicitly initialized, you can just return here

I won't change this, due to Swarm code policy. Please tell me if you disagree.

mortelli · 2019-09-06T13:56:54Z

swap/swap.go

+		log.Info("Using the provided chequebook", "chequebookAddr", chequebookAddr)
+	} else {
+		if err := s.Deploy(context.Background(), s.backend); err != nil {
+			return err


I won't change this, due to Swarm code policy. Please tell me if you disagree.

mortelli · 2019-09-06T13:57:13Z

swap/swap.go

+		}
+		log.Info("New SWAP contract deployed", "contract info", s.DeploySuccess())
+	}
+	return nil


return or declare the function return as error instead of err error

yes get rid of named return value here

Got rit of named return

mortelli

i am keeping my approval review, but i have made some additional (minor) comments.

be sure to run the linter, the test suite, and rebase/merge with master to have this ready to merge after any change.

santicomp2014

The PR #1720 will be affected by these changes.
I will wait until you get the PR Accepted, so I can pull these changes and continue with my PR.

LGTM.

zelig · 2019-09-08T17:43:44Z

swap/swap.go

+		}
+		log.Info("New SWAP contract deployed", "contract info", s.DeploySuccess())
+	}
+	return nil


yes get rid of named return value here

zelig · 2019-09-08T17:45:41Z

swap/swap.go

+	}
+	s.contract, err = contract.InstanceAt(address, s.backend)
+	if err != nil {
+		return err


get rid of named paramater.
@mortelli our code policy is to write return arguments explicitly even if we have named return params

zelig · 2019-09-08T17:47:29Z

swarm.go

-			}
-			log.Info("New SWAP contract deployed", "contract info", s.swap.DeploySuccess())
-		}
+		s.swap.StartChequebook(s.config.Contract)


oops, you swallow the error returned here.

Oops, I'll throw it out again!

zelig · 2019-09-09T19:49:30Z

swap/swap.go

+	}
+	s.contract, err = contract.InstanceAt(address, s.backend)
+	if err != nil {
+		return err


this is fine. I also like sometimes that vars are initialised,
the policy is more about not having bare return-s if the function has return values.
it makes things more readable

* 'master' of github.com:ethersphere/swarm: network: terminate Hive connect goroutine on Stop (ethersphere#1740) Incentives rpc test (ethersphere#1733) swarm, swap: pass chequebook address at start-up (ethersphere#1718)

swarm: use passed --chequebook param, swap: don't use chequebookAddre…

9f3b920

…ss on swap.new, create function NewInstanceAt

Eknir requested review from holisticode and mortelli September 2, 2019 07:55

Eknir added 2 commits September 2, 2019 09:58

Merge branch 'master' into chequebookAddress

2ce640e

swap: updated call to swap.New in tests

596fffa

Eknir added the incentives label Sep 2, 2019

Eknir marked this pull request as ready for review September 3, 2019 07:10

Eknir requested review from skylenet and vojtechsimetka September 3, 2019 07:12

vojtechsimetka approved these changes Sep 3, 2019

View reviewed changes

Eknir added the ready for review label Sep 3, 2019

ralph-pichler reviewed Sep 3, 2019

View reviewed changes

swarm.go Outdated Show resolved Hide resolved

Eknir added 2 commits September 3, 2019 17:08

swarm: swapEnabled err checking

6b0b0ab

swap: updated comment at NewInstanceAt

fae4caf