-
Notifications
You must be signed in to change notification settings - Fork 112
swap: chequebook persistence #1797
Changes from 21 commits
7f45609
6692bae
a6354de
cf0ba7e
56daf95
6e391f0
0c260c3
632de27
662233f
949c5dc
5d293c6
ac2160e
c148730
419a06f
5f00a9c
7408362
a07c5c1
ec9260d
4b4e3f7
6759d92
b8ff0fc
6b98755
b7f98ac
26d9467
f9e1837
e2e8edc
e414145
170878d
bbf2f9b
a5fd51a
bf2cad0
2e53e83
675c797
2441747
5d7484d
047422c
98771d6
42aaca0
286cf36
27e2220
aca25a2
8b1400d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -96,6 +80,24 @@ type simpleContract struct { | |
address common.Address | ||
} | ||
|
||
// Deploy deploys an instance of the underlying contract and returns its address | ||
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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please use nouns for variable names There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -64,6 +64,7 @@ type Swap struct { | |
honeyPriceOracle HoneyOracle // oracle which resolves the price of honey (in Wei) | ||
paymentThreshold int64 // honey amount at which a payment is triggered | ||
disconnectThreshold int64 // honey amount at which a peer disconnects | ||
backendNetworkID uint64 // the backend network ID to which the node is connected | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we need to be clear this is a blockchain ID, either through the comment or the variable name. unless you guys think that this is understood by the term There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The term We have two options according to me:
Maybe I am overseeing an option? I would vote for option 2 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I agree to option 2. |
||
} | ||
|
||
// Owner encapsulates information related to accessing the contract | ||
|
@@ -119,7 +120,7 @@ func swapRotatingFileHandler(logdir string) (log.Handler, error) { | |
} | ||
|
||
// new - swap constructor without integrity check | ||
func new(logpath string, stateStore state.Store, prvkey *ecdsa.PrivateKey, backend contract.Backend, disconnectThreshold uint64, paymentThreshold uint64) *Swap { | ||
func new(logpath string, stateStore state.Store, prvkey *ecdsa.PrivateKey, backend contract.Backend, disconnectThreshold uint64, paymentThreshold uint64, backendNetworkID uint64) *Swap { | ||
auditLog = newLogger(logpath) | ||
return &Swap{ | ||
store: stateStore, | ||
|
@@ -130,6 +131,7 @@ func new(logpath string, stateStore state.Store, prvkey *ecdsa.PrivateKey, backe | |
disconnectThreshold: int64(disconnectThreshold), | ||
paymentThreshold: int64(paymentThreshold), | ||
honeyPriceOracle: NewHoneyPriceOracle(), | ||
backendNetworkID: backendNetworkID, | ||
} | ||
} | ||
|
||
|
@@ -143,28 +145,60 @@ func New(logpath string, dbPath string, prvkey *ecdsa.PrivateKey, backendURL str | |
// 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. message is kind of redundant here, there's 2 occurrences
but it's not a major gripe There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. Log could be: There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 disconnectThreshold <= paymentThreshold { | ||
return nil, fmt.Errorf("swap init error: disconnect threshold lower or at payment threshold. DisconnectThreshold: %d, PaymentThreshold: %d", disconnectThreshold, 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) | ||
} | ||
networkID, err := backend.NetworkID(context.TODO()) | ||
if err != nil { | ||
return nil, fmt.Errorf("swap init error: error retrieving networkID from backendURL: %v", err) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same as the previous error message redundancy comment i made There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree |
||
} | ||
var usedBeforeAtNetwork uint64 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 😄 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (same here) |
||
} | ||
// read from database | ||
if err == nil { | ||
usedBefore = true | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
return nil, fmt.Errorf("swap init error: error writing current backend networkID to statestore: %v", err) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (same here) |
||
} | ||
log.Info("SWAP initialized on backend network ID", "ID", networkID.Uint64()) | ||
} | ||
|
||
return new( | ||
logpath, | ||
stateStore, | ||
prvkey, | ||
backend, | ||
disconnectThreshold, | ||
paymentThreshold), nil | ||
paymentThreshold, | ||
networkID.Uint64(), | ||
), nil | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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):
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. for clarity i would just name this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not very fond of the But there may even better ones. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
@@ -436,51 +470,58 @@ 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 | ||
} | ||
log.Info("Using the provided chequebook", "chequebookAddr", chequebookAddr) | ||
} else { | ||
if err := s.Deploy(context.Background()); err != nil { | ||
return err | ||
// StartChequebook start the chequebook, taking into account the chequebookAddress passed in by the user and the chequebook addresses saved on the node's database | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changed |
||
func (s *Swap) StartChequebook(chequebookAddrFlag common.Address) 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{}) { | ||
chequebook, err := s.Deploy(context.TODO()) | ||
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) | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
auditLog.Info("Using the chequebook", "chequebookAddr", address, "networkID", s.backendNetworkID) | ||
// 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) (common.Address, error) { | ||
opts := bind.NewKeyedTransactor(s.owner.privateKey) | ||
// initial topup value | ||
opts.Value = big.NewInt(int64(s.params.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()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why was this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would like to know as well There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The log, when we deploy a chequebook will be:
Given that we have these two logs, I thought it would be unnecessary to also have the log
If you don't agree, I will change it There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated. |
||
|
||
return err | ||
log.Info("deploying new swap", "owner", opts.From.Hex()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch |
||
return s.deployLoop(opts, s.owner.address, defaultHarddepositTimeoutDuration) | ||
} | ||
|
||
// deployLoop repeatedly tries to deploy the swap contract . | ||
|
@@ -491,8 +532,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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The simpleSwap instance is redundant here because the I think this design is cleaner because the code now clearly does what it says it does: Furthermore, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will remove the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
log.Warn("can't send chequebook deploy tx", "try", try, "error", err) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
|
@@ -503,3 +544,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) | ||
} |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be updated now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
startChequebook
is now called in swap.New(). By doing so, the prompt is now clean and initializing SWAP actually initializes SWAP fully (beforeswap.contract
was nil, which could lead to nasty nil-pointer dereference errors)