-
Notifications
You must be signed in to change notification settings - Fork 112
Conversation
…oad from state at boot-up
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.
first pass through the code for me. i've left some comments, but i would like to request a more in-depth explanation for this network ID business, if possible :)
i think i am missing some of the high level logic that i need to understand in order to review the code better.
what's the idea behind using and storing network IDs, specifically?
thanks
@mortelli , thanks for the code review. The networkID is needed in the statestore, because otherwise if a node saves a chequebook when connecting to (say) Rinkeby, the node will attempt to load this same chequebook when the node connects to (say) Mainnet. The contract is obviously not deployed on mainnet, so we have to add the networkID as a key in the database. If not, the node will error on
Based on review from @ralph-pichler, I added the same logic to the key for |
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.
A couple of minor things to look at please.
I am a bit nervous with this adding of a network ID again, but I can't pinpoint it and can't name any real problem for now
…de it impossible to connect with one statestore to multiple Ethereum network IDs
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.
I don't think your networkID
logic works because you're using different userBefore keys.
My idea was:
- there is one
usedBeforeNetworkId
key - on start check if it exists
- if it exists compare it against the current one and abort if it doesn't match
- if it doesn't exist save it as
usedBeforeNetworkId
and continue
The rest of the PR looks good to me.
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.
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 fromSwarm.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 intoNew
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.
contracts/swap/swap.go
Outdated
// 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 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)
swap/swap.go
Outdated
@@ -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 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.
swap/swap.go
Outdated
if err != nil { | ||
return nil, fmt.Errorf("swap init error: error retrieving networkID from backendURL: %v", err) | ||
} | ||
var usedBeforeAtNetwork uint64 |
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.
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()
)
swap/swap.go
Outdated
} | ||
// read from database | ||
if err == nil { | ||
usedBefore = true |
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.
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 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.
swap/swap.go
Outdated
@@ -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 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
swap/swap_test.go
Outdated
opts := bind.NewKeyedTransactor(swap.owner.privateKey) | ||
opts.Value = big.NewInt(int64(swap.params.InitialDepositAmount)) | ||
opts.Context = ctx | ||
|
||
swap.contract, _, err = cswap.Deploy(opts, swap.backend, swap.owner.address, defaultHarddepositTimeoutDuration) | ||
address, _, _, err := cswap.Deploy(opts, swap.backend, swap.owner.address, defaultHarddepositTimeoutDuration) |
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.
Why is this change?
@@ -109,19 +108,6 @@ func TestNewSwarm(t *testing.T) { | |||
} | |||
}, | |||
}, | |||
{ |
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.
Me too.
In fact on first glance (I only did a quick check) it seems as it is not possible to pass the initial amount via config variable anymore, and this tests would probably catch it!
…k initial networkID in checkNetworkID
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.
Great job!
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.
Nodes don't start anymore on empty datadirs, this needs to be fixed before merge.
As for the issue with StartChequebook
I don't like it but if @holisticode and @mortelli think it's ok I'll accept that.
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.
LGTM
One final request: Please change the title from persistance to persistence |
Rework from #1790 , now with a shorter git-history
closes #1796
Closes the first bullet point of #1739
swap
This PR is a feature which is important to help users in rebooting their swap-enabled swarm. Without the feature, users would have to manually collect from the logs the address of their previously deployed chequebook and provide this the next time via command line/config/env.
Test scenario's by hand:
Node boots up and deploys chequebook:
/home/rinke/go/src/github.com/ethersphere/swarm/build/bin/swarm --swap --swap-backend-url https://rinkeby.infura.io/v3/<infure_key> --bzznetworkid=5 --password ~/.ethereum/swarm/standardPassword.txt
Node boots up and uses previously deployed chequebook (from test scenario 1):
/home/rinke/go/src/github.com/ethersphere/swarm/build/bin/swarm --swap --swap-backend-url https://rinkeby.infura.io/v3/<infure_key> --bzznetworkid=5 --password ~/.ethereum/swarm/standardPassword.txt
Node boots up and uses previously deployed chequebook (from scenario 1 and 2), with chequebookAddress passed-in
/home/rinke/go/src/github.com/ethersphere/swarm/build/bin/swarm --swap --swap-backend-url https://rinkeby.infura.io/v3/<infure_key> --bzznetworkid=5 --password ~/.ethereum/swarm/standardPassword.txt --swap-chequebook <address_from_before>
Node does not boot up, with chequebookAddress passed-in. Reason: other chequebook used before
/home/rinke/go/src/github.com/ethersphere/swarm/build/bin/swarm --swap --swap-backend-url https://rinkeby.infura.io/v3/<infure_key> --bzznetworkid=5 --password ~/.ethereum/swarm/standardPassword.txt --swap-chequebook <different_address>
Remove data file
swap.db
Node boots up, with chequebook address passed-in
/home/rinke/go/src/github.com/ethersphere/swarm/build/bin/swarm --swap --swap-backend-url https://rinkeby.infura.io/v3/<infure_key> --bzznetworkid=5 --password ~/.ethereum/swarm/standardPassword.txt --swap-chequebook <address_from_before>
Remove file
swap.db
Node does not boot up, with chequebookAddress passed-in. Reason: contract validation failed
/home/rinke/go/src/github.com/ethersphere/swarm/build/bin/swarm --swap --swap-backend-url https://rinkeby.infura.io/v3/<infure_key> --bzznetworkid=5 --password ~/.ethereum/swarm/standardPassword.txt --swap-chequebook <different_address>
Node does not boot up when attempting to connect with a different than Ethereum Network ID
/home/rinke/go/src/github.com/ethersphere/swarm/build/bin/swarm --swap --swap-backend-url https://ropsen.infura.io/v3/<infure_key> --bzznetworkid=5 --password ~/.ethereum/swarm/standardPassword.txt --swap-chequebook <different_address>