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 5 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 an already existing chequebook contract at address and sets chequebookAddr
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
20 changes: 15 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,21 @@ 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
if err := cswap.ValidateCode(context.Background(), s.backend, address); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't we call verifyContract instead of ValidateCode directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice one. Indeed that would be better!

Copy link
Contributor Author

@Eknir Eknir Sep 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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:

  1. remove ValidateCode and always call verifyContract directly
  2. always call ValidateCode for consistency's sake

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

thank you for putting this down for discussion.

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

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
}
if err := s.swap.NewInstanceAt(address, s.backend); 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 {
if err := s.swap.Deploy(context.Background(), s.backend, s.config.Path); 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