swap: refactor lastReceivedCheque, lastSentCheque, balances to peer #1725
Conversation
ralph-pichler
force-pushed
the
swap-peer
branch
3 times, most recently
from
6d700c2 to
7b8194d
Compare
There was a problem hiding this comment.
this PR is definitely moving in the right direction, great work so far 👍
i did not pay much attention to the tests, just made sure that they were passing. i will for sure check back on them on a later (non-draft) iteration.
although it probably will be hard, we should try to find a way to make sure that the peer locks are behaving the way we want them to... not sure that we can capture this with a unit test, though.
e.g.: call Add for the same peer twice and check that the execution of these routines is not concurrent, but check that it is when calling Add for 2 different peers simultaneously.
ralph-pichler
force-pushed
the
swap-peer
branch
2 times, most recently
from
6d69949 to
feda7ac
Compare
swap/peer.go
Outdated
| return fmt.Errorf("error while creating cheque: %v", err) | ||
| } | ||
|
|
||
| log.Info("sending cheque", "honey", cheque.Honey, "cumulativePayout", cheque.ChequeParams.CumulativePayout, "beneficiary", cheque.Beneficiary, "contract", cheque.Contract) |
| @@ -134,17 +136,22 @@ func (s *Swap) removePeer(p *Peer) { | |||
| delete(s.peers, p.ID()) | |||
| } | |||
|
|
|||
| func (s *Swap) addPeer(p *Peer) { | |||
| func (s *Swap) addPeer(protoPeer *protocols.Peer, beneficiary common.Address, contractAddress common.Address) (*Peer, error) { | |||
There was a problem hiding this comment.
I would keep addPeer as it was and put the peer initialisation in the run function.
There was a problem hiding this comment.
I would like to keep peer initialisation and adding it to the swaps peer mapping in one common function as creating a Peer without adding it to the map is always a mistake (this only happens at one place in the main code but would lead to a lot of duplicate code in the tests). I used to have this in a separate separate function but @holisticode recommended doing this in addPeer (see #1725 (comment)).
ralph-pichler
force-pushed
the
swap-peer
branch
from
f609d58 to
1c58a7c
Compare
ralph-pichler
force-pushed
the
swap-peer
branch
from
b5e5e7a to
296863b
Compare
| // Caller must be careful that the same resources aren't concurrently read and written by multiple routines | ||
| func (p *Peer) createCheque() (*Cheque, error) { | ||
| var cheque *Cheque | ||
| var err error |
There was a problem hiding this comment.
if i understand our coding standards correctly, we could use named parameters here so as not to have to initialize these variables (as long as we don't use naked returns).
however, i don't know if this is still acceptable for relatively long functions like this one.
There was a problem hiding this comment.
you dont need this declarations, just put a comma after cheque on line 138
| s.peersLock.RLock() | ||
| defer s.peersLock.RUnlock() | ||
| peer, ok := s.peers[id] | ||
| return peer, ok | ||
| peer := s.peers[id] |
There was a problem hiding this comment.
why are we removing the ok check here? What if the peer is not in the map? Do you just want to force the caller to check for nil?
There was a problem hiding this comment.
Yes, this was suggested here (#1725 (comment))
* 'master' of github.com:ethersphere/swarm: all: support go 1.13 change to testing/flag packages (ethersphere#1710) swap: refactor lastReceivedCheque, lastSentCheque, balances to peer (ethersphere#1725)
This PR
lastSentChequetoPeerbalancestoPeerbalancewhen thePeeris createdpeerswhen it is createdbalanceLock,chequesLockandaccountingLocksendChequeandcreateChequetoPeercloses #1604