-
-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor(peer) some _minor_ safety features #419
Conversation
License: MIT Signed-off-by: Brian Tiger Chow <[email protected]>
License: MIT Signed-off-by: Brian Tiger Chow <[email protected]>
License: MIT Signed-off-by: Brian Tiger Chow <[email protected]>
for safety! use mockpeer.WithID methods to create peers in tests License: MIT Signed-off-by: Brian Tiger Chow <[email protected]>
License: MIT Signed-off-by: Brian Tiger Chow <[email protected]>
} | ||
|
||
type peerstore struct { | ||
sync.RWMutex | ||
peers ds.Datastore | ||
data map[string]Peer // key is string(ID) |
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 want to keep this as a Datastore. this abstraction doesnt hurt anything here. while it is not a TODO to make it persistent today, we have the option to.
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.
How about if we decide to go the persistence route, I'll nominate myself to do the dirty work?
(datastore impl about 50 lines of superfluous unmarshalling, type casting, and error checking)
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.
what unmarshaling?
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.
sorry. somehow visualized the cast as an unmarshal in my mind.
referring to these blocks of code
k := u.Key(i).DsKey()
val, err := p.peers.Get(k)
switch err {
// some other datastore error
default:
return nil, err
// not found, construct it ourselves, add it to datastore, and return.
case ds.ErrNotFound:
// TODO(brian) kinda dangerous, no? If ID is invalid and doesn't
// correspond to an actual valid peer ID, this peerstore will return an
// instantiated peer value, allowing the error to propagate. It might
// be better to nip this at the bud by returning nil and making the
// client manually add a Peer. To keep the peerstore in control, this
// can even be a peerstore method that performs cursory validation.
//
// Potential bad case: Suppose values arrive from untrusted providers
// in the DHT.
peer := &peer{id: i}
if err := p.peers.Put(k, peer); err != nil {
return nil, err
}
return peer, nil
// no error, got it back fine
case nil:
peer, ok := val.(*peer)
if !ok {
return nil, errors.New("stored value was not a Peer")
}
return peer, nil
}
k := peer.Key().DsKey()
val, err := p.peers.Get(k)
switch err {
// some other datastore error
default:
return nil, err
// not found? just add and return.
case ds.ErrNotFound:
err := p.peers.Put(k, peer)
return peer, err
// no error, already here.
case nil:
peer2, ok := val.(Peer)
if !ok {
return nil, errors.New("stored value was not a Peer")
}
if peer == peer2 {
return peer, nil
}
// must do some merging.
peer2.Update(peer)
return peer2, 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.
the merging is no longer there because we always create it here. sure, it's a bit of noise. can rebase to move the ds stuff to one commit so it can be reverted easily?
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.
can rebase to move the ds stuff to one commit?
That's how I roll 🐯 1e00779
License: MIT Signed-off-by: Brian Tiger Chow <[email protected]>
@jbenet addressed all comments RFCR |
LGTM |
refactor(peer) some _minor_ safety features
…com/multiformats/go-multiaddr-0.2.0 build(deps): bump github.com/multiformats/go-multiaddr from 0.1.2 to 0.2.0
This PR cleans up the peer package, making it a tiny bit safer to use.
mockpeer
package.(This refactor was performed because of work #416 and a promise I made a long time ago)