bitpay / bitcore-p2p

Interface to the bitcoin P2P network for bitcore
MIT License
80 stars 276 forks source link

Extensible Messages #48

Closed braydonf closed 9 years ago

braydonf commented 9 years ago
coveralls commented 9 years ago

Coverage Status

Coverage increased (+5.9%) to 100.0% when pulling ede5f0b60c1662f71af964b95eaf5b20ddcca176 on braydonf:feature/extensibility into ae93c147b7d09e1de9e6530a07d197e43d06903c on bitpay:master.

coveralls commented 9 years ago

Coverage Status

Coverage increased (+5.9%) to 100.0% when pulling f17bbf5d6f4fa4bc5f213f9622a59f5423ff79f3 on braydonf:feature/extensibility into ae93c147b7d09e1de9e6530a07d197e43d06903c on bitpay:master.

coveralls commented 9 years ago

Coverage Status

Coverage increased (+5.9%) to 100.0% when pulling 8f2d0089df225e29ca7d20d7062f6c68d56b1988 on braydonf:feature/extensibility into ae93c147b7d09e1de9e6530a07d197e43d06903c on bitpay:master.

eordano commented 9 years ago

Amazing work, great!

There are a few nits on the tests names but I'd approve this PR as is. I'll review it more in-deep this afternoon

braydonf commented 9 years ago

Two questions:

  1. Should we remove all of the fromObject methods on the message command constructors, since they nolonger seem necesssary ?
  2. Should the magicNumber for messages be stored as a buffer, so it's the same as Network. And writing this is nolonger needed: bitcore.Networks.defaultNetwork.networkMagic.readUInt32LE(0) ?
eordano commented 9 years ago
  1. I wouldn't, it may be useful at some point if we implement .toObject (dunno, maybe somebody interested in sending serialized messages in json?)
  2. Either way it's fine by me. The only verbosity you'd save is the readUINt32LE, right? I'm slightly more concerned about every message doing magicNumber = bitcore.Networks....
braydonf commented 9 years ago
  1. Yeah, except that new Message() can be used for those purposes is most (all currently) cases. For JSON it would be fromJSON() that would then call new Message(objectFromJSON).
  2. Only a slight difference, it may be possible to configure Message when required (similar to how the commands are being required with options) so that it magicNumber would be defined at that level instead.

Edit: Took a look at implementing #2 And think the current solution is better.

coveralls commented 9 years ago

Coverage Status

Coverage increased (+5.9%) to 100.0% when pulling c0e3bdb1908fb228149a4dd020f1b48e63779e9a on braydonf:feature/extensibility into ae93c147b7d09e1de9e6530a07d197e43d06903c on bitpay:master.

eordano commented 9 years ago

LGTM, let's merge this

maraoz commented 9 years ago

Great work in general, left some small comments. Please fix/reply and I'll be happy to merge this :)

coveralls commented 9 years ago

Coverage Status

Coverage increased (+5.9%) to 100.0% when pulling 34c38466f7c95366a117a9bee8a30904b7bb8d10 on braydonf:feature/extensibility into ae93c147b7d09e1de9e6530a07d197e43d06903c on bitpay:master.