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

swap: chequebook persistence #1797

Merged
merged 42 commits into from
Sep 26, 2019
Merged
Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
42 commits
Select commit Hold shift + click to select a range
7f45609
swap, contracts/swap: save used chequebook to state and attempt to l…
Eknir Sep 20, 2019
6692bae
swap: fix broken test
Eknir Sep 20, 2019
a6354de
swap: add networkID as a parameter to save/load used chequebooks from…
Eknir Sep 23, 2019
cf0ba7e
implemented NetworkID for swapTestBackend
Eknir Sep 23, 2019
56daf95
swap: added tests covering the connecting to an existing chequebook
Eknir Sep 23, 2019
6e391f0
swap: made testChequeContract2 not the same as testChequeContract
Eknir Sep 23, 2019
0c260c3
swap: saved networkID on SWAP object
Eknir Sep 23, 2019
632de27
swap: add networkID prefix to balances, received cheque and sent cheque
Eknir Sep 23, 2019
662233f
swap:renaming and comments
Eknir Sep 23, 2019
949c5dc
removed testNewSwarm:withSwap testcase because of dependency on backe…
Eknir Sep 23, 2019
5d293c6
swap: removed networkID from balances prefix
Eknir Sep 23, 2019
ac2160e
swap: more elaborate startChequebook tests, testNetworkID under testB…
Eknir Sep 24, 2019
c148730
swap: renamed networkID to backendNetworkID. Removed debug prints
Eknir Sep 24, 2019
419a06f
swap: removed redundant whitespace
Eknir Sep 24, 2019
5f00a9c
swap: fix misspell
Eknir Sep 24, 2019
7408362
swap: removed Ethereum networkID identifiers from statestore keys, ma…
Eknir Sep 25, 2019
a07c5c1
swap: error messages and comments
Eknir Sep 25, 2019
ec9260d
swap: fix tests and fix logical error with regards to storing the bac…
Eknir Sep 25, 2019
4b4e3f7
swap: updated comments on sentChequeKey and receivedChequeKey
Eknir Sep 25, 2019
6759d92
swap: remove usedBeforeAtNetworkPrefix
Eknir Sep 25, 2019
b8ff0fc
Merge branch 'master' into incentive-chequebook_persistance1
Eknir Sep 25, 2019
6b98755
Merge branch 'master' into incentive-chequebook_persistance1
Eknir Sep 25, 2019
b7f98ac
swap: prompt to startChequebook
Eknir Sep 25, 2019
26d9467
swap: initialize chequebook in swap.New
Eknir Sep 26, 2019
f9e1837
swap: make tests pass again
Eknir Sep 26, 2019
e2e8edc
swap: update comments, logs, variable names
Eknir Sep 26, 2019
e414145
swap: usedBefore fix
Eknir Sep 26, 2019
170878d
swap: updated expected error in tests
Eknir Sep 26, 2019
bbf2f9b
swap: remove auditlog from protocol_test.go
Eknir Sep 26, 2019
a5fd51a
swap: updated comment at swap.New
Eknir Sep 26, 2019
bf2cad0
swap: made comment to match godoc specification
Eknir Sep 26, 2019
2e53e83
swap: updated comment for SWAP deployment
Eknir Sep 26, 2019
675c797
swap: removed backendNetworkID from swap
Eknir Sep 26, 2019
2441747
swap: remove networkID from simulatedBackend
Eknir Sep 26, 2019
5d7484d
swap: update statestore keys
Eknir Sep 26, 2019
047422c
swap: more changes from feedback
Eknir Sep 26, 2019
98771d6
swap: assign variables in if-statement in swap-new and save chequeboo…
Eknir Sep 26, 2019
42aaca0
swap: fix reading network ID on initial boot
Eknir Sep 26, 2019
286cf36
swap: use chainID instead of networkID
Eknir Sep 26, 2019
27e2220
swap: put receivers back
Eknir Sep 26, 2019
aca25a2
swap: made test pass again
Eknir Sep 26, 2019
8b1400d
swap: change log message
Eknir Sep 26, 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
34 changes: 18 additions & 16 deletions contracts/swap/swap.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,22 +45,6 @@ type Backend interface {
TransactionReceipt(ctx context.Context, txHash common.Hash) (*types.Receipt, error)
}

// Deploy deploys an instance of the underlying contract and returns its `Contract` abstraction
func Deploy(auth *bind.TransactOpts, backend bind.ContractBackend, owner common.Address, harddepositTimeout time.Duration) (Contract, *types.Transaction, error) {
addr, tx, s, err := contract.DeploySimpleSwap(auth, backend, owner, big.NewInt(int64(harddepositTimeout)))
c := simpleContract{instance: s, address: addr}
return c, tx, err
}

// InstanceAt creates a new instance of a contract at a specific address.
// It assumes that there is an existing contract instance at the given address, or an error is returned
// This function is needed to communicate with remote Swap contracts (e.g. sending a cheque)
func InstanceAt(address common.Address, backend bind.ContractBackend) (Contract, error) {
simple, err := contract.NewSimpleSwap(address, backend)
c := simpleContract{instance: simple, address: address}
return c, err
}

// Contract interface defines the methods exported from the underlying go-bindings for the smart contract
type Contract interface {
// CashChequeBeneficiary cashes the cheque by the beneficiary
Expand Down Expand Up @@ -96,6 +80,24 @@ type simpleContract struct {
address common.Address
}

// Deploy deploys an instance of the underlying contract and returns its address
Copy link
Contributor

Choose a reason for hiding this comment

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

we are returning more than an address here.

in general, i don't think it's necessary for comments to indicate that an error can be returned for example, but in this case we are returning 4 variables, so i think a more descriptive comment should be written in this case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be updated now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good work tackling this bit of complex matter.

There are a couple of issues though. The most important ones:

  • Moving it to StartChequebook now gives the problem of other go routines messing up the prompt. I don't think we should overlook that. The reason is that you do all this initialization from Swarm.Start(). As I have commented elsewhere, this should actually be the correct way to initialize services, but we don't use it like this. I would suggest to move everything back into New to avoid these issues.
  • It seems to me that it is not possible anymore to provide an initial amount via config flag, can you check?

I like the idea of removing the InitialDepositAmount altogether from the Params, as it really only is used once and it doesn't need to be stored really. However make sure that the param can be set in any case.

  • startChequebook is now called in swap.New(). By doing so, the prompt is now clean and initializing SWAP actually initializes SWAP fully (before swap.contract was nil, which could lead to nasty nil-pointer dereference errors)
  • In the code from yesterday evening, we could still set the initialDepositAmount (verified this by hand, please check the test scenario's by hand in the PR description). It is still possible after the refactor of this morning (verified it by the same scenarios)

func Deploy(auth *bind.TransactOpts, backend bind.ContractBackend, owner common.Address, harddepositTimeout time.Duration) (common.Address, *types.Transaction, *contract.SimpleSwap, error) {
addr, tx, simpleSwap, err := contract.DeploySimpleSwap(auth, backend, owner, big.NewInt(int64(harddepositTimeout)))
return addr, tx, simpleSwap, err
}

// InstanceAt creates a new instance of a contract at a specific address.
// It assumes that there is an existing contract instance at the given address, or an error is returned
// This function is needed to communicate with remote Swap contracts (e.g. sending a cheque)
func InstanceAt(address common.Address, backend Backend) (Contract, error) {
simple, err := contract.NewSimpleSwap(address, backend)
Copy link
Contributor

Choose a reason for hiding this comment

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

please use nouns for variable names

Copy link
Contributor

Choose a reason for hiding this comment

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

Mind you he just moved what was existing, so it was like this before.

But I am ok changing these things too (this is why careful with moving things around without changing them, it becomes unclear IF something changed and by who)

Copy link
Contributor Author

@Eknir Eknir Sep 26, 2019

Choose a reason for hiding this comment

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

Updated. I changed the order because there were functions before type definitions, which was very confusing for me. I agree that this makes it unclear if something is changed and what, but I think it was justified this time.

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

if err != nil {
return nil, err
}
c := simpleContract{instance: simple, address: address}
return c, err
}

// CashChequeBeneficiary cashes the cheque on the blockchain and blocks until the transaction is mined.
func (s simpleContract) CashChequeBeneficiary(auth *bind.TransactOpts, backend Backend, beneficiary common.Address, cumulativePayout *big.Int, ownerSig []byte) (*CashChequeResult, *types.Receipt, error) {
tx, err := s.instance.CashChequeBeneficiary(auth, beneficiary, cumulativePayout, ownerSig)
Expand Down
179 changes: 112 additions & 67 deletions swap/swap.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ type Swap struct {
backend contract.Backend // the backend (blockchain) used
owner *Owner // contract access
params *Params // economic and operational parameters
backendNetworkID uint64 // backend network ID used by SWAP to settle payments
contract swap.Contract // reference to the smart contract
honeyPriceOracle HoneyOracle // oracle which resolves the price of honey (in Wei)
}
Expand All @@ -75,10 +76,9 @@ type Owner struct {

// Params encapsulates param
type Params struct {
LogPath string // optional audit log path
InitialDepositAmount uint64 // initial deposit amount for the chequebook
PaymentThreshold int64 // honey amount at which a payment is triggered
DisconnectThreshold int64 // honey amount at which a peer disconnects
LogPath string // optional audit log path
PaymentThreshold int64 // honey amount at which a payment is triggered
DisconnectThreshold int64 // honey amount at which a peer disconnects
}

// newLogger returns a new logger
Expand Down Expand Up @@ -120,14 +120,15 @@ func swapRotatingFileHandler(logdir string) (log.Handler, error) {
}

// new - swap constructor without integrity check
func new(stateStore state.Store, prvkey *ecdsa.PrivateKey, backend contract.Backend, params *Params) *Swap {
func new(stateStore state.Store, prvkey *ecdsa.PrivateKey, backend contract.Backend, params *Params, backendNetworkID uint64) *Swap {
auditLog = newLogger(params.LogPath)
return &Swap{
store: stateStore,
peers: make(map[enode.ID]*Peer),
backend: backend,
owner: createOwner(prvkey),
params: params,
backendNetworkID: backendNetworkID,
holisticode marked this conversation as resolved.
Show resolved Hide resolved
honeyPriceOracle: NewHoneyPriceOracle(),
}
}
Expand All @@ -142,50 +143,57 @@ func New(dbPath string, prvkey *ecdsa.PrivateKey, backendURL string, params *Par
// initialize the balances store
stateStore, err := state.NewDBStore(filepath.Join(dbPath, "swap.db"))
if err != nil {
return nil, fmt.Errorf("swap init error: %s", err)
return nil, fmt.Errorf("swap init error: error initializing statestore: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

message is kind of redundant here, there's 2 occurrences error and :, i suggest something like:

swap init error while initializing state store

but it's not a major gripe

Copy link
Contributor

Choose a reason for hiding this comment

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

Remember, only log//add an error if it has not been logged before (so useful to go and check), and/or if it actually adds to the available error message.

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 , I agree with your proposal here and it hold for all error messages in this code block.
@holisticode , I think we add information to the log here.

Log could be: swap init error while initializing state store: leveldb/storage: open dir/swap.db not a directory

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the original error is fine enough (remember that the log will show where the error happened, so it's easy to spot), the additional text is superfluous for me.

But I don't mind keeping it as-is.

}
if params.DisconnectThreshold <= params.PaymentThreshold {
return nil, fmt.Errorf("swap init error: disconnect threshold lower or at payment threshold. DisconnectThreshold: %d, PaymentThreshold: %d", params.DisconnectThreshold, params.PaymentThreshold)
}
backend, err := ethclient.Dial(backendURL)
if err != nil {
return nil, fmt.Errorf("swap init error: error connecting to Ethereum API %s: %s", backendURL, err)
return nil, fmt.Errorf("swap init error: error connecting to Ethereum API %s: %v", backendURL, err)
}

// we may not need this check, and we could maybe even get rid of this constant completely
if params != nil && params.InitialDepositAmount == 0 {
// need to prompt user for initial deposit amount
// if 0, can not cash in cheques
prompter := console.Stdin

// ask user for input
input, err := prompter.PromptInput("Please provide the amount in Wei which will deposited to your chequebook upon deployment: ")
if err != nil {
return nil, err
}
// check input
val, err := strconv.ParseInt(input, 10, 64)
networkID, err := backend.NetworkID(context.TODO())
if err != nil {
return nil, fmt.Errorf("swap init error: error retrieving networkID from backendURL: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

same as the previous error message redundancy comment i made

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree

}
var usedBeforeAtNetwork uint64
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a very strange variable name in my opinion, but you know how i feel about settling on names, so take this just as a passing comment 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, these names are strange - what was used before?

I suggest something like firstConnectedBlockchain or something (Network is not descriptive enough and could be misleading, even if the call on the blockchain is called NetworkID() )

var usedBefore bool
err = stateStore.Get(usedBeforeAtNetworkKey, &usedBeforeAtNetwork)
// error reading from database
if err != nil && err != state.ErrNotFound {
return nil, fmt.Errorf("swap init error: error querying usedBeforeAtNetwork from statestore: %v", err)
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)

}
// read from database
if err == nil {
usedBefore = true
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this safe enough? What if the state store went away?

That would be of course a user error. And in that case all balances and cheques would be gone anyway, which means that the node starts afresh.

I am just asking you to think of what this could mean for the complete initialization process here. I guess there is no problem whatsoever, please confirm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the user corrupts it's state store many things can go wrong. Let's say it would only delete the entry about the networkID. In this case, the user might connect on a different network, where he will (among other potential errors) not be able to cash the cheques which should be cashable according to the statestore.

}
// if used before on a different network, abort
ralph-pichler marked this conversation as resolved.
Show resolved Hide resolved
if usedBefore && usedBeforeAtNetwork != networkID.Uint64() {
return nil, fmt.Errorf("swap init error: statestore previously used on different backend network. Used before on network: %d, Attempting to connect on network %d", usedBeforeAtNetwork, networkID)
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)

}
// put the networkID in the statestore
if !usedBefore {
err = stateStore.Put(usedBeforeAtNetworkKey, networkID.Uint64())
if err != nil {
// maybe we should provide a fallback here? A bad input results in stopping the boot
return nil, fmt.Errorf("Conversion error while reading user input: %v", err)
return nil, fmt.Errorf("swap init error: error writing current backend networkID to statestore: %v", err)
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)

}
log.Info("Chequebook initial deposit amount: ", "amount", val)
params.InitialDepositAmount = uint64(val)
log.Info("SWAP initialized on backend network ID", "ID", networkID.Uint64())
}

swap := new(
return new(
stateStore,
prvkey,
backend,
params)

return swap, nil
params,
networkID.Uint64(),
), nil
Copy link
Contributor

Choose a reason for hiding this comment

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

as a comment for the function in general: i think all of the network id logic is needed, but it kind of makes the code convoluted. if possible, i would extract all of this logic in a separate function.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, this whole initialization process is long and not very readable.

I also think it could be compressed. Not well verified but you could check if this works (and if it is correct):

if err == nil {
  if  usedBeforeAtNetwork != network.Uint64() {
  ...
   }
} else {
   stateStore.Put()
}
...

Maybe there is even a more elegant way.

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 put the logic in a separate function

}

const (
balancePrefix = "balance_"
sentChequePrefix = "sent_cheque_"
receivedChequePrefix = "received_cheque_"
balancePrefix = "balance_"
sentChequePrefix = "sent_cheque_"
receivedChequePrefix = "received_cheque_"
usedChequebookKey = "used_chequebook"
usedBeforeAtNetworkKey = "used_before_at_network"
Copy link
Contributor

Choose a reason for hiding this comment

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

for clarity i would just name this used_network, i think it's consistent with used_chequebook

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not very fond of the usedxxx naming.
I prefer firstConnectedBlockchain or even just firstBlockchain.

But there may even better ones.

Copy link
Contributor Author

@Eknir Eknir Sep 26, 2019

Choose a reason for hiding this comment

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

Changed both to

	connectedChequebookKey = "connected_chequebook"
	connectedBlockchainKey = "connected_blockchain"

)

// returns the store key for retrieving a peer's balance
Expand Down Expand Up @@ -217,11 +225,6 @@ func createOwner(prvkey *ecdsa.PrivateKey) *Owner {
}
}

// DeploySuccess is for convenience log output
func (s *Swap) DeploySuccess() string {
return fmt.Sprintf("contract: %s, owner: %s, deposit: %v, signer: %x", s.GetParams().ContractAddress.Hex(), s.owner.address.Hex(), s.params.InitialDepositAmount, s.owner.publicKey)
}

// Add is the (sole) accounting function
// Swap implements the protocols.Balance interface
func (s *Swap) Add(amount int64, peer *protocols.Peer) (err error) {
Expand Down Expand Up @@ -457,51 +460,83 @@ func (s *Swap) getContractOwner(ctx context.Context, address common.Address) (co
return contr.Issuer(nil)
}

// StartChequebook deploys a new instance of a chequebook if chequebookAddr is empty, otherwise it wil bind to an existing instance
func (s *Swap) StartChequebook(chequebookAddr common.Address) error {
if chequebookAddr != (common.Address{}) {
if err := s.BindToContractAt(chequebookAddr); err != nil {
return err
func promptInitialDepositAmount() (uint64, error) {
// need to prompt user for initial deposit amount
// if 0, can not cash in cheques
prompter := console.Stdin

// ask user for input
input, err := prompter.PromptInput("Please provide the amount in Wei which will deposited to your chequebook upon deployment: ")
if err != nil {
return 0, err
}
// check input
val, err := strconv.ParseInt(input, 10, 64)
if err != nil {
// maybe we should provide a fallback here? A bad input results in stopping the boot
return 0, fmt.Errorf("Conversion error while reading user input: %v", err)
}
return uint64(val), nil
}

mortelli marked this conversation as resolved.
Show resolved Hide resolved
// StartChequebook start the chequebook, taking into account the chequebookAddress passed in by the user and the chequebook addresses saved on the node's database
Copy link
Contributor

Choose a reason for hiding this comment

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

should be starts

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

func (s *Swap) StartChequebook(chequebookAddrFlag common.Address, initialDepositAmount uint64) error {
chequebookToUse, err := s.loadChequebook()
// error reading from disk
if err != nil && err != state.ErrNotFound {
return fmt.Errorf("Error reading previously used chequebook: %s", err)
holisticode marked this conversation as resolved.
Show resolved Hide resolved
}
// nothing written to state disk before, no flag provided: deploying new chequebook
if err == state.ErrNotFound && chequebookAddrFlag == (common.Address{}) {
var toDeposit = initialDepositAmount
if toDeposit == 0 {
toDeposit, err = promptInitialDepositAmount()
if err != nil {
return err
}
}
log.Info("Using the provided chequebook", "chequebookAddr", chequebookAddr)
} else {
if err := s.Deploy(context.Background()); err != nil {
return err
chequebook, err := s.Deploy(context.TODO(), toDeposit)
if err != nil {
return fmt.Errorf("Error deploying chequebook: %v", err)
}
log.Info("New SWAP contract deployed", "contract info", s.DeploySuccess())
holisticode marked this conversation as resolved.
Show resolved Hide resolved
chequebookToUse = chequebook
}
return nil
}

// BindToContractAt binds an instance of an already existing chequebook contract at address and sets chequebookAddr
func (s *Swap) BindToContractAt(address common.Address) (err error) {
// read from state, but provided flag is not the same
if err == nil && (chequebookAddrFlag != common.Address{} && chequebookAddrFlag != chequebookToUse) {
return fmt.Errorf("Attempting to connect to provided chequebook, but different chequebook used before")
}
if chequebookAddrFlag != (common.Address{}) {
chequebookToUse = chequebookAddrFlag
}
auditLog.Info("Using the chequebook", "chequebookAddr", chequebookToUse)
holisticode marked this conversation as resolved.
Show resolved Hide resolved
return s.bindToContractAt(chequebookToUse)
}
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: we have too much store/config code here and much less code actually starting the chequebook. i think it's better to separate this if possible

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 had a look and simplified where possible. If you think we can still simplify it, I propose to make an issue to tackle this


// BindToContractAt binds to an instance of an already existing chequebook contract at address
func (s *Swap) bindToContractAt(address common.Address) (err error) {
// validate whether address is a chequebook
if err := contract.ValidateCode(context.Background(), s.backend, address); err != nil {
return fmt.Errorf("contract validation for %v failed: %v", address, err)
return fmt.Errorf("contract validation for %v failed: %v", address.Hex(), err)
}
// get the instance and save it on swap.contract
s.contract, err = contract.InstanceAt(address, s.backend)
if err != nil {
return err
}
return nil
// saving chequebook
return s.saveChequebook(address)
}

// Deploy deploys the Swap contract and sets the contract address
func (s *Swap) Deploy(ctx context.Context) error {
func (s *Swap) Deploy(ctx context.Context, initialDepositAmount uint64) (common.Address, error) {
opts := bind.NewKeyedTransactor(s.owner.privateKey)
// initial topup value
opts.Value = big.NewInt(int64(s.params.InitialDepositAmount))
opts.Value = big.NewInt(int64(initialDepositAmount))
opts.Context = ctx

auditLog.Info("deploying new swap", "owner", opts.From.Hex())
address, err := s.deployLoop(opts, s.owner.address, defaultHarddepositTimeoutDuration)
if err != nil {
auditLog.Error("unable to deploy swap", "error", err)
return err
}
auditLog.Info("swap deployed", "address", address.Hex(), "owner", opts.From.Hex())
Copy link
Contributor

Choose a reason for hiding this comment

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

why was this Info removed? can we not have this log in the deployLoop call, at least?

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 like to know as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The log, when we deploy a chequebook will be:

auditLog.Info("deploying new swap", "owner", opts.From.Hex(), "deposit", opts.Value) (line 538)
auditLog.Info("Using the chequebook", "chequebookAddr", chequebookToUse) (line 512)

Given that we have these two logs, I thought it would be unnecessary to also have the log

auditLog.Info("swap deployed", "address", address.Hex(), "owner", opts.From.Hex())

If you don't agree, I will change it

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that "using the chequebook" isn't very helpful (is the chequebook address in hex?)

First "deploying new..." and then "swap deployed" makes more sense to me. It clearly shows the user that the deployment is being attempted and then that it succeeded.

Maybe we could move "using the chequebook" to a debug log.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.
We now have a log for deploying,
auditLog.Info("Deploying new swap", "owner", opts.From.Hex(), "deposit", opts.Value)
log for deployed
auditLog.Info("Deployed chequebook", "contract address", contract.ContractParams().ContractAddress.Hex(), "deposit", toDeposit, "owner", owner.address)
amd a seperate log for if we are re-using a previously deployed chequebook
auditLog.Info("bound to chequebook", "chequebookAddr", address)


return err
log.Info("deploying new swap", "owner", opts.From.Hex(), "deposit", opts.Value)
return s.deployLoop(opts, s.owner.address, defaultHarddepositTimeoutDuration)
}

// deployLoop repeatedly tries to deploy the swap contract .
Expand All @@ -512,8 +547,8 @@ func (s *Swap) deployLoop(opts *bind.TransactOpts, owner common.Address, default
time.Sleep(deployDelay)
}

if s.contract, tx, err = contract.Deploy(opts, s.backend, owner, defaultHarddepositTimeoutDuration); err != nil {
auditLog.Warn("can't send chequebook deploy tx", "try", try, "error", err)
if addr, tx, _, err = contract.Deploy(opts, s.backend, owner, defaultHarddepositTimeoutDuration); 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.

how are we getting away with ignoring a variable here? makes the design suspicious if we can

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, this is production code, not test code, let's not ignore, and if it needs to be ignored, explain why

Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't worry too much about it now. This will be changed with the factory PR anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

contract.deploy returns an address, the transaction identifier, and a simpleSwap instance.

The simpleSwap instance is redundant here because the startChequebook function ends with a call to bindToContractAt, which sets the contract instance on swap.

I think this design is cleaner because the code now clearly does what it says it does: deploy deploys and only deploys, bindToContractAt binds the contract to SWAP and nothing else.

Furthermore, startChequebook is easy to reason about now: the function body gets an address of a chequebook (either by deploying or pass-in), it ends with binding (which both verifies the deployed code and saves the chequebook for later usage).

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 will remove the simpleSwap instance as a return variable from contract.deploy

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 it not as described above. We are now returning the instance from the whole deploy structure, and use this instance as a return variable from startChequebook.

log.Warn("can't send chequebook deploy tx", "try", try, "error", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

auditLog

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

continue
}
if addr, err = bind.WaitDeployed(opts.Context, s.backend, tx); err != nil {
Expand All @@ -524,3 +559,13 @@ func (s *Swap) deployLoop(opts *bind.TransactOpts, owner common.Address, default
}
return addr, err
}

func (s *Swap) loadChequebook() (common.Address, error) {
var chequebook common.Address
err := s.store.Get(usedChequebookKey, &chequebook)
return chequebook, err
}

func (s *Swap) saveChequebook(chequebook common.Address) error {
return s.store.Put(usedChequebookKey, chequebook)
}
Loading