-
Notifications
You must be signed in to change notification settings - Fork 526
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
P2P State Channel M2 Evaluation #1229
base: master
Are you sure you want to change the base?
Conversation
## General Notes | ||
There are some TODOs left in the code. Some seem to be minor. There is one in the StateManager.ts in the playTransaction() function. | ||
|
||
```//TODO! calculate who didn't sign so we stop signing their blocks``` |
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.
This TODO here is an optimization to not cooperate with peers who didn't cooperate with you - if you still cooperate with them you just potentially save them some fees :D since they might have all signatures and won't have to post calldata on-chain as part of the liveness logic
Actually the code for this is already there - https://github.com/peer3to/state-channels-plus/blob/43189afeeb92da7d0586b2441e4540af059ec67f/src/StateManager.ts#L510 - I just had other critical things to implement
Many TODOs are there as guidance for future functionality
|
||
E.g. ``rechallengeRecusrisve()``in ``DisputeHandler.ts`` | ||
|
||
There is also a lot of functions without proper inline documentation. Please add more inline documentation. |
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've fixed that typo and added more inline docs
|
||
## Article | ||
Usually this delivery shall be a fully fledged article which covers various aspects of your project. Something to be posted on plattforms like medium. This is too short to be an article in my opinion. The current text is closer to a twitter post than an article. | ||
|
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.
Sure I'll update the text and let you know :D
No description provided.