swap: exchange chain id on handshake #1913
Conversation
ralph-pichler
added
in progress
incentives
builds on open PR
ralph-pichler
changed the title
ralph-pichler
force-pushed
the
swap-drop-peers
branch
from
52d648d
to
6be043a
Compare
ralph-pichler
force-pushed
the
swap-drop-peers
branch
2 times, most recently
from
4559b5d
to
2c2f6a7
Compare
ralph-pichler
removed
the
builds on open PR
ralph-pichler
force-pushed
the
swap-drop-peers
branch
from
2c2f6a7
to
d4a9044
Compare
|
|
||
| // sign the cheque | ||
| cheque.Signature, err = cheque.Sign(swap.owner.privateKey) | ||
| if err == nil { |
There was a problem hiding this comment.
?
Isn't a TestDisconnected returning an err if it does not find the expected disconnect? So wouldn't here err be nil if it actually found the provided Disconnect, which is saying stay connected, which is what you wanted to test?
There was a problem hiding this comment.
Yes, on a disconnect err would be nil. Which is an error because because this is where we we want to make sure that there is no disconnect. (This function is copied from the network tests btw).
There was a problem hiding this comment.
It's a bit counter-intuitive to pass a Disconnect object when not expecting a disconnect...Can't this just be called with an empty object, which would signal that we don't expect a disconnect?
Then err would return nil only if there would be no disconnect.
If that's not possible, I guess that's the way to do it and we keep it (I understand you copied it from elsewhere)
There was a problem hiding this comment.
Afaik there is no other way to check if the node is disconnected or not. (It also would not be possible, if we passed nil, then the function would not know which peer to check).
I would keep the function as it is, so that we remain consistent with the handshake test style from the other protocols.
swap/protocol.go
Outdated
| @@ -34,6 +34,9 @@ var ( | |||
| // ErrEmptyAddressInSignature is used when the empty address is used for the chequebook in the handshake | |||
| ErrEmptyAddressInSignature = errors.New("empty address in handshake") | |||
|
|
|||
| // ErrDifferentChainId is used when the chain id exchanged during the handshake does not match | |||
| ErrDifferentChainId = errors.New("different chain id") | |||
There was a problem hiding this comment.
var ErrDifferentChainId should be ErrDifferentChainID -- go-lint
There was a problem hiding this comment.
I noticed that in the linter output when developing. Looks like I forgot to actually commit that. Interesting that the CI didn't catch this. Should be fixed now.
swap/types.go
Outdated
| @@ -36,6 +36,7 @@ type Cheque struct { | |||
|
|
|||
| // HandshakeMsg is exchanged on peer handshake | |||
| type HandshakeMsg struct { | |||
| ChainID uint64 | |||
There was a problem hiding this comment.
Please leave a comment here, specifying that this is the chainID of the Ethereum network
| @@ -98,6 +101,10 @@ func (s *Swap) verifyHandshake(msg interface{}) error { | |||
| return ErrEmptyAddressInSignature | |||
| } | |||
|
|
|||
| if handshake.ChainID != s.chainID { | |||
There was a problem hiding this comment.
Could you update the comment to the function verifyHandshake? Right now it says that it is only verifying the chequebook address transmitted
swap/protocol_test.go
Outdated
| newSwapHandshakeMsg(common.Address{}, 1234), | ||
| &p2ptest.Disconnect{ | ||
| Peer: protocolTester.Nodes[0].ID(), | ||
| Error: errors.New("Handshake error: Message handler error: (msg code 0): empty address in handshake"), |
There was a problem hiding this comment.
Can you somehow create this error by including the (exported) error messages from the different protocols? Such that when if the content of the error message is changed in one protocol, it does not make our test fail?
For example, the expected error message here is a concatenation of:
protocols.ErrHandshake + protocols.ErrHandler + swap.ErrDifferentChainID.
| } | ||
|
|
||
| // creates a test exchange for the handshakes | ||
| func HandshakeMsgExchange(lhs, rhs *HandshakeMsg, id enode.ID) []p2ptest.Exchange { |
There was a problem hiding this comment.
Why is this function exported?
| @@ -130,7 +131,7 @@ func swapRotatingFileHandler(logdir string) (log.Handler, error) { | |||
| } | |||
|
|
|||
| // newSwapInstance is a swap constructor function without integrity checks | |||
| func newSwapInstance(stateStore state.Store, owner *Owner, backend contract.Backend, params *Params, chequebookFactory contract.SimpleSwapFactory) *Swap { | |||
| func newSwapInstance(stateStore state.Store, owner *Owner, backend contract.Backend, chainID uint64, params *Params, chequebookFactory contract.SimpleSwapFactory) *Swap { | |||
There was a problem hiding this comment.
It would be great to have the order of function arguments the same as the order of the functions in the swap struct
There was a problem hiding this comment.
This already wasn't the case prior to the PR
There was a problem hiding this comment.
That being said I want the backend and chain id next to each other since they kind of belong together. So it should be changed in the struct.
This PR