Sign block locally #12
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #12 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 29 31 +2
Lines 435 455 +20
Branches 76 70 -6
=========================================
+ Hits 435 455 +20 ☔ View full report in Codecov by Sentry. |
lib/modules/blocks/blocks.ts
Outdated
There was a problem hiding this comment.
Does it make sense to keep the option to create the block with the RPC server? With your current changes, there is no options to do so, right?
There was a problem hiding this comment.
I've thought about it, but I don't think so.
I really don't see the point of using the node to sign the block, it can lead to errors and private key leakage.
The late goal of this feature is to implement the ability to generate local work, where for the work it makes perfect sense to provide the choice to the user, because the local machine or server may not have the hardware to handle this kind of job. And there is no possibility of the private key being leaked.
But if you have an argument against it, I'm open to it.
There was a problem hiding this comment.
The only reason I see is feature compatibility. If a user has a need to create the block with the RPC server we want to give them the option to do so. Maybe we can provide some warning regard the issue with a private key, but in the end it's a feature the Nano protocol supports, so why should we not include it?
But I will leave this decision up to you. For now we can also just keep it out and see if an issue is raised in the future
tgerboui
force-pushed
the
feat/10-sign-block-locally
branch
from
3d9f3c3 to
6dda4c6
Compare
| @@ -212,13 +222,19 @@ describe('Wallet class', () => { | |||
| }; | |||
| const hashInfo = { amount: '2000000000000000000000000000000' }; | |||
| wallet.info = jest.fn().mockResolvedValue(info); | |||
| workMock.generate | |||
| .mockResolvedValueOnce({ work: 'work' } as any) | |||
| .mockResolvedValueOnce({ work: 'work2' } as any); | |||
| blocksMock.info.mockResolvedValue(hashInfo as any); | |||
There was a problem hiding this comment.
I would consider any bad practice, because the type checking TypeScript provides gets disabled. Maybe you could remove it?
| @@ -382,7 +403,8 @@ describe('Wallet class', () => { | |||
| const newRepresentative = randomNanoAddress(); | |||
| const info = { balance: '1000', frontier: 'frontierHash' }; | |||
| wallet.info = jest.fn().mockResolvedValue(info); | |||
| blocksMock.create.mockResolvedValue({ hash: 'createdChangeHash' } as any); | |||
| workMock.generate.mockResolvedValue({ work: 'work' } as any); | |||
| blocksMock.create.mockReturnValueOnce({ hash: 'createdChangeHash' } as any); | |||
There was a problem hiding this comment.
Same here with any (see above)
In this PR blocks are now hashed and signed locally.
Work generation (call at the rpc) is now separated in a work module.
The
hashandsignmethods have been added in the blocks module.blocks.createuses theses functions withwork.generateto format a new block.Solve #10