Messages vs MessageBase

[17Q4MX2hmktmpuUKHFuoRmS5MfB5XPbhod]

The @aynik Messages class appears to implement an interface that's tightly coupled to the blockchain abstraction, which makes testing hard. Additionally, there's a number of odd object couplings inside the message class (see the #create's ChatMessage reference), and the class was not intended to be inherited from by messages, as per the ruby implementation. It would by my proposal that the Message class be migrated to the recently stubbed/created MessageBase implementation, and that the incumbent Blockchain class be renamed "BlockchainSpv" (or something similar)

@aynik - what are your thoughts here? If you look at the buyer class, this is how I believe these mesages should look whenever possible. Have a better idea?

[ghost]

the class was not intended to be inherited from by messages, as per the ruby implementation

I think I may have analysed this part incorrectly, what is the real purpose of it? I thought it was inheritance because I saw some polymorphism going on in other parts.

the incumbent Blockchain class be renamed "BlockchainSpv"

I agree with this in general terms, I think we should go forward and plugin-ize it, because I see alternative implementations using btcd (@krisives was interested in this part) or block explorers.

Let's first address the Messages issue and then we will follow with Blockchain, I will read now the code you submitted in depth.

[ghost]

I'm thinking that it may be a misunderstanding with the naming? I've checked the ruby code and Communication does inherit from MessageBase, well that should be the same as ChatMessage inheriting from Message shouldn't it?

[17Q4MX2hmktmpuUKHFuoRmS5MfB5XPbhod]

what is the real purpose of it?

I consider all non value-transfer transactions to be "Messages". These message types are outlined in the whitepaper, and this MessageBase class was modeled (roughly) after the ActiveRecord::Base class. Classes which inherit from MessageBase inherit the methods used to define the parameters of messages, as well as the methods needed to persist and load messages. (#find(), #save(), etc)

Communication does inherit from MessageBase, well that should be the same as ChatMessage inheriting from Message shouldn't it?

Indeed. Though it seems that ChatMessage has a bit too much logic in the class that should be moved into Message. I believe Message and MessageBase should be merged, I don't care what name we give that class (We can retain the name Message).

I believe the Messages class should be deprecated, and that find should move into the Message class itself. We may want to emulate the design of the orm2 library a bit more closely, and rather than return instances of (say) ChatMessage from finders, we return Hashes that are decorated with a mixin. Does this make sense? Thoughts?

[ghost]

Ok, more or less as I thought but I greatly appreciate the clarification.

it seems that ChatMessage has a bit too much logic in the class that should be moved into Message

You're right, I thought the time to do this will come soon after finishing the chat subcommand, at the time I wasn't sure about this because I did not inspect the other subcommands and message types, but I agree with this 100%.

I believe Message and MessageBase should be merged

Let's do this next, I'd like to keep Message as the name of the base class if you don't mind. Maybe the best is that you port all that you have to src/messages and after I can do all the other refactoring we're discussing here.

I believe the Messages class should be deprecated, and that find should move into the Message class itself.

Yeah, at first I thought I would instantiate this to represent a collection of messages but as I completed chat it was more and more obvious messages don't need to be represented as a single object, hence the Messages class was left as a namespace just implementing find. This method can be renamed and relocated to the messages module itself, so using it is more intuitive (messages.findAll).

The orm2 collection and instance interface is not bad, but the library itself has given me some problems, I'll say let's use its method names for continuity but not it's ordering syntax and the other stuff.

we return Hashes that are decorated with a mixin

What do you mean with Hashes? TxIds?

Regarding caching right there, I still think is not the best. Drivers for bitcoind RPC or block explorers would not benefit from this as much as SPV, so I think most of the caching should be done there (and we'll have a much cleaner 'messages'). The decision to cache there was made to reuse dropzone_ruby's cache, so moving it from there will be soft-breaking (backwards compatible but transactions table will be no longer used) compatibility though.

[17Q4MX2hmktmpuUKHFuoRmS5MfB5XPbhod]

Take a look at the newest commit and see what you think? Look at the implementation of the src/buyer.js and see if that wouldn't be a good model for the Communication class to follow. It's heavily inspired by the ruby implementation, but should allow for much easier testing and provides better interfaces for other connections down the line.

Regarding the hashes - I abandoned that suggestion. Ignore this.

If you're agreeable to the proposed interfaces, I'd like to rename MessageBase to Message, and ask that much of the existing Message code be inserted into a BitcoinSpv "connection". Would you like to do this? If so, I'll begin work on the other Message types, starting with their test code.

[ghost]

Referencing these here:

https://github.com/17Q4MX2hmktmpuUKHFuoRmS5MfB5XPbhod/node-dropzone/commit/66953e3224fea7e6bf0d0c1e95b262d8d580671f#commitcomment-13989425 https://github.com/17Q4MX2hmktmpuUKHFuoRmS5MfB5XPbhod/node-dropzone/commit/55f19ae7eda27d6acefc4d9dcbb92dc5ebed1557#commitcomment-13990592 https://github.com/17Q4MX2hmktmpuUKHFuoRmS5MfB5XPbhod/node-dropzone/commit/0c369a0fd239fb82000b9bbc90dc62d7793a0c53#commitcomment-13990676 https://github.com/17Q4MX2hmktmpuUKHFuoRmS5MfB5XPbhod/node-dropzone/commit/2a6ed4d1a5035eea3707f78f88dacb8f96759e51#commitcomment-13990720

[17Q4MX2hmktmpuUKHFuoRmS5MfB5XPbhod]

See my response to those comments?