Skip to content
This repository has been archived by the owner on Aug 2, 2021. It is now read-only.

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

Merged
merged 11 commits into from
Sep 10, 2019
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
9f3b920
swarm: use passed --chequebook param, swap: don't use chequebookAddre…
Eknir Sep 2, 2019
2ce640e
Merge branch 'master' into chequebookAddress
Eknir Sep 2, 2019
596fffa
swap: updated call to swap.New in tests
Eknir Sep 2, 2019
6b0b0ab
swarm: swapEnabled err checking
Eknir Sep 3, 2019
fae4caf
swap: updated comment at NewInstanceAt
Eknir Sep 3, 2019
ff854d1
swarm, swap: error handling and renaming
Eknir Sep 4, 2019
1a8d089
swap, swarm: rename InstanceAt to BindToContractAt
Eknir Sep 4, 2019
53d1398
Merge branch 'master' into chequebookAddress
Eknir Sep 4, 2019
9d7f449
swarm, swap: refactor => chequebook setup in swap, remove verifyContract
Eknir Sep 5, 2019
274c1c5
swarm, swap: err returning and err handling
Eknir Sep 9, 2019
81338c9
Merge branch 'master' into chequebookAddress
Eknir Sep 9, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 21 additions & 6 deletions swap/swap.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,14 +86,14 @@ func NewParams() *Params {
}

// New - swap constructor
func New(stateStore state.Store, prvkey *ecdsa.PrivateKey, contract common.Address, backend contract.Backend) *Swap {
func New(stateStore state.Store, prvkey *ecdsa.PrivateKey, backend contract.Backend) *Swap {
return &Swap{
store: stateStore,
balances: make(map[enode.ID]int64),
cheques: make(map[enode.ID]*Cheque),
peers: make(map[enode.ID]*Peer),
backend: backend,
owner: createOwner(prvkey, contract),
owner: createOwner(prvkey),
params: NewParams(),
paymentThreshold: DefaultPaymentThreshold,
disconnectThreshold: DefaultDisconnectThreshold,
Expand Down Expand Up @@ -127,12 +127,11 @@ func keyToID(key string, prefix string) enode.ID {
}

// createOwner assings keys and addresses
func createOwner(prvkey *ecdsa.PrivateKey, contract common.Address) *Owner {
func createOwner(prvkey *ecdsa.PrivateKey) *Owner {
pubkey := &prvkey.PublicKey
return &Owner{
mortelli marked this conversation as resolved.
Show resolved Hide resolved
privateKey: prvkey,
publicKey: pubkey,
Contract: contract,
address: crypto.PubkeyToAddress(*pubkey),
}
}
Expand Down Expand Up @@ -532,6 +531,11 @@ func (s *Swap) verifyContract(ctx context.Context, address common.Address) error
return contract.ValidateCode(ctx, s.backend, address)
}

// SetChequebookAddr sets the chequebook address
func (s *Swap) SetChequebookAddr(chequebookAddr common.Address) {
holisticode marked this conversation as resolved.
Show resolved Hide resolved
s.owner.Contract = chequebookAddr
mortelli marked this conversation as resolved.
Show resolved Hide resolved
}

// getContractOwner retrieve the owner of the chequebook at address from the blockchain
func (s *Swap) getContractOwner(ctx context.Context, address common.Address) (common.Address, error) {
contr, err := contract.InstanceAt(address, s.backend)
Expand All @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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).

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

func (s *Swap) NewInstanceAt(address common.Address, backend swap.Backend) error {
mortelli marked this conversation as resolved.
Show resolved Hide resolved
c, err := contract.InstanceAt(address, backend)
holisticode marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return err
Copy link
Contributor

Choose a reason for hiding this comment

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

since err is implicitly initialized, you can just return here

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

}
s.contract = c
s.SetChequebookAddr(address)
return nil
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}

// Deploy deploys the Swap contract and sets the contract address
func (s *Swap) Deploy(ctx context.Context, backend swap.Backend, path string) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

opts := bind.NewKeyedTransactor(s.owner.privateKey)
// initial topup value
Expand All @@ -555,7 +570,7 @@ func (s *Swap) Deploy(ctx context.Context, backend swap.Backend, path string) er
log.Error("unable to deploy swap", "error", err)
return err
}
s.owner.Contract = address
s.SetChequebookAddr(address)
log.Info("swap deployed", "address", address.Hex(), "owner", opts.From.Hex())

return err
Expand Down
2 changes: 1 addition & 1 deletion swap/swap_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -502,7 +502,7 @@ func newBaseTestSwap(t *testing.T, key *ecdsa.PrivateKey) (*Swap, string) {
}
log.Debug("creating simulated backend")

swap := New(stateStore, key, common.Address{}, testBackend)
swap := New(stateStore, key, testBackend)
return swap, dir
}

Expand Down
23 changes: 18 additions & 5 deletions swarm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ func NewSwarm(config *api.Config, mockStore *mock.NodeStore) (self *Swarm, err e
return nil, err
}
// create the accounting objects
self.swap = swap.New(swapStore, self.privateKey, self.config.Contract, self.backend)
self.swap = swap.New(swapStore, self.privateKey, self.backend)
// start anonymous metrics collection
self.accountingMetrics = protocols.SetupAccountingMetrics(10*time.Second, filepath.Join(config.Path, "metrics.db"))
}
Expand Down Expand Up @@ -374,11 +374,24 @@ func (s *Swarm) Start(srv *p2p.Server) error {
log.Info("Updated bzz local addr", "oaddr", fmt.Sprintf("%x", newaddr.OAddr), "uaddr", fmt.Sprintf("%s", newaddr.UAddr))

if s.config.SwapEnabled {
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{}) {
holisticode marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

address := s.config.Contract
err := cswap.ValidateCode(context.Background(), s.backend, address)
ralph-pichler marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return fmt.Errorf("Provided address not a chequebook smart-contract: %v", err)
holisticode marked this conversation as resolved.
Show resolved Hide resolved
mortelli marked this conversation as resolved.
Show resolved Hide resolved
}
err = s.swap.NewInstanceAt(address, s.backend)
holisticode marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return fmt.Errorf("Could not set the instance at provided cheqeubook: %v", err)
mortelli marked this conversation as resolved.
Show resolved Hide resolved
holisticode marked this conversation as resolved.
Show resolved Hide resolved
}
log.Info("Using the provided chequebook", "chequebookAddr", address)
} else {
err := s.swap.Deploy(context.Background(), s.backend, s.config.Path)
if err != nil {
return fmt.Errorf("Unable to deploy swap contract: %v", err)
holisticode marked this conversation as resolved.
Show resolved Hide resolved
}
log.Info("SWAP contract deployed", "contract info", s.swap.DeploySuccess())
holisticode marked this conversation as resolved.
Show resolved Hide resolved
}
log.Info("SWAP contract deployed", "contract info", s.swap.DeploySuccess())
} else {
log.Info("SWAP disabled: no chequebook set")
}
Expand Down