search

shierve / nem-voting

nem library that implements voting functions in typescript into an npm module
MIT License
9 stars 3 forks source link

code review #3

Open ghost opened 6 years ago

ghost commented 6 years ago

Reviewing the source code I've noticed that the Poll class is abstract and then you have two implementations UnbroadcastedPoll & BroadcastedPoll

https://github.com/shierve/nem-voting/blob/616ecd142a584fb1f0c6168d0da167ad74b0c662/src/poll.ts#L79-L133

Most of methods have a lot of side effects, a domain model shouldn't talk with the network, because you are coupling the model with the infrastructure implementation, making it harder to test and change later.

The pattern you have applied is the Active Record Pattern, but due how nem-library is designed, it doesn't fit well, since we use the Repository Pattern.

On the other side, the method validate seems to contain all the logic to know if it's valid or not, right? Seems like the method with the responsibility of anouncing the transaction doesn't called it before announcing, is it responsibility of the developer to call the validate method before announcing? IMO, the Poll constructor should check the integrity of the data, and in case of be invalid, throw an exception.

https://github.com/shierve/nem-voting/blob/616ecd142a584fb1f0c6168d0da167ad74b0c662/src/poll.ts#L296

Reviewing the vote method, I have seen that you ask the Account instance, because it contains the private key, it could be a risk for the user. Reason? It is not in your case, but in the future, people will start creating libraries for NEM, and it will attract developers with bad intentions. We aim to not accept libraries that asks for the Account class for security reasons.

Sign a transaction and announce it should be done always outside the library, since it is not its responsibility.

https://github.com/shierve/nem-voting/blob/616ecd142a584fb1f0c6168d0da167ad74b0c662/src/poll.ts#L319

shierve commented 6 years ago

I will look into the first issue. The issue with validation is that when implementing this library I made some changes to how a poll is generated, so old polls will not be validated properly with this function. I am waiting for NanoWallet to release its latest version which is using this library, and then I will apply this automatically, but only for polls that are created after a certain block. About asking for an account, what do you propose? I don't know how to do this without asking for the account.