wire_refactor by Vib-UX · Pull Request #187 · node-lightning/node-lightning

Vib-UX · 2021-08-05T02:59:54Z

fixes #183 wire: refactor message types to use @node-lightning/bitcoin types

packages/wire/__tests__/messages/ClosingSignedMessage.spec.ts

codecov-commenter · 2021-08-05T21:16:44Z

Codecov Report

Merging #187 (314fb00) into main (de8afec) will decrease coverage by 0.03%.
The diff coverage is 86.95%.

❗ Current head 314fb00 differs from pull request most recent head d792690. Consider uploading reports for the commit d792690 to get more accurate results

@@            Coverage Diff             @@
##             main     #187      +/-   ##
==========================================
- Coverage   93.71%   93.67%   -0.04%     
==========================================
  Files         179      179              
  Lines        4693     4696       +3     
  Branches      547      547              
==========================================
+ Hits         4398     4399       +1     
- Misses        163      165       +2     
  Partials      132      132

Impacted Files	Coverage Δ
packages/bitcoin/lib/Value.ts	`85.71% <0.00%> (-14.29%)`	⬇️
packages/wire/lib/messages/ChannelUpdateMessage.ts	`72.60% <69.23%> (ø)`
...h/lib/deserialize/channel-settings-from-message.ts	`100.00% <100.00%> (ø)`
packages/wire/lib/messages/AcceptChannelMessage.ts	`96.15% <100.00%> (+0.07%)`	⬆️
packages/wire/lib/messages/ClosingSignedMessage.ts	`100.00% <100.00%> (ø)`
packages/wire/lib/messages/OpenChannelMessage.ts	`96.96% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update de8afec...d792690. Read the comment docs.

Vib-UX · 2021-08-05T22:06:18Z

Not sure why the build error? 🤔

bmancini55

First pass is looking good. Couple minors and wanted your thoughts on a few properties in channel_update

packages/wire/lib/messages/ChannelUpdateMessage.ts

packages/wire/lib/messages/ClosingSignedMessage.ts

packages/wire/lib/messages/OpenChannelMessage.ts

bmancini55

Apologies for the review delay. Just a minor change to naming, otherwise looks good!

packages/bitcoin/lib/Value.ts

bmancini55 · 2021-08-11T14:37:46Z

Not sure why the build error? thinking

Check to make sure npm run build works in the wire project. Sometimes interdependencies can be tricky as well, the error is happening inside gossip-rocksdb project.

From the project root you can run npm run bootstrap and it should reproduce the error locally.

Vib-UX · 2021-08-11T23:02:04Z

Not sure why the build error? thinking

Check to make sure npm run build works in the wire project. Sometimes interdependencies can be tricky as well, the error is happening inside gossip-rocksdb project.

From the project root you can run npm run bootstrap and it should reproduce the error locally.

Locally its showing success.

bmancini55 · 2021-08-12T18:01:53Z

Whew, ok, that took a bit to unpack what is going on.

The short answer is that by adding types from the bitcoin project to output from the wire project, the TypeScript build output no longer has a shared root path. Previously all types were part of /wire/lib which is not included in the dist path. The package.json's main path of /wire/dist/index.js worked. Now because of the bitcoin types, the main entry for the package.json must be set to /wire/dist/wire/lib/index.js. This is observerable if you

navigate to the wire project folder, cd packages/wire
remove the dist folder, rm -rf dist
rebuild the project, npm run build
view the contents of the dist, ls dist

You'll see that it now contains two directories, bitcoin and wire. And wire/lib/index.js is the entry point for the library.

So... to repro the build error:

from the root of the node-lightning project
delete all build data (I'm going to make a new issue to ensure this happens prior to builds)

find ./packages -name "dist" -exec rm -rf {} \;
find ./packages -name "node_modules" -exec rm -rf {};

Rerun bootstrap, npm run bootstrap
It will fail with the same error in the build.

You can now fix the wire project by changing package.json so that main points to dist/wire/lib/index.js.

Rerunning npm run bootstrap at the project root will reveal the actual build errors that still exist inside the graph project!

bmancini55 · 2021-08-12T18:17:36Z

Ah.. it's much simpler than that. Should have done a more thorough code review first!

In a number of files you import Value via the path ../../../bitcoin/lib/Value (sometimes VS Code imports via relative paths instead of the projects). This path needs to be changed to @node-lightning/core and you'll be able to reproduce the build errors.

Vib-UX · 2021-08-12T22:13:18Z

Ah.. it's much simpler than that. Should have done a more thorough code review first!

In a number of files you import Value via the path ../../../bitcoin/lib/Value (sometimes VS Code imports via relative paths instead of the projects). This path needs to be changed to @node-lightning/core and you'll be able to reproduce the build errors.

Ooh, 😮 didn't think that would be the case. 👍

bmancini55

Need to make sure the tests for GraphMaanger are still valid. You'll either need to modify the helper to generate the Value types in the spec file or modify channelSettingsFromMessage to null check and return 0 if it's null.

bmancini55 · 2021-08-13T14:19:20Z

packages/graph/lib/deserialize/channel-settings-from-message.ts

-    instance.htlcMaximumMsat = msg.htlcMaximumMsat;
-    instance.feeBaseMsat = msg.feeBaseMsat;
-    instance.feeProportionalMillionths = msg.feeProportionalMillionths;
+    instance.htlcMinimumMsat = msg.htlcMinimumMsat.msats;


Even though this shouldn't be null, let's perform a null check on these properties. This will allow the test for GraphManager to succeed.

null check 👍

bmancini55 · 2021-08-13T14:20:49Z

packages/graph/__tests__/graph-manager.spec.ts


            gossipEmitter.emit("message", createMsg());
-            gossipEmitter.emit("message", createUpdateMsg(0));
+            // gossipEmitter.emit("message", createUpdateMsg(0));


This shouldn't be commented out. Need to either to null check inside channelSettingsFromMessage or fix the helper createUpdateMsg to generate an update message with instantiated Value types.

It was getting bit trickier to handle so temporarily commented it out, it was showing TypeError: Cannot read property msats of undefined when working with createUpdateMsg.

Sure will try to work around it.

Yep, that's a null ref exception. It's cause by crateUpdateMsg not populating all of the message properties https://github.com/Vib-UX/node-lightning/blob/fe82b0731df63180d43b00e64ee4db43aec2692f/packages/graph/__tests__/graph-manager.spec.ts#L102.

bmancini55 · 2021-08-13T14:20:51Z

packages/graph/__tests__/graph-manager.spec.ts


            gossipEmitter.emit("message", createMsg());
-            gossipEmitter.emit("message", createUpdateMsg(1));
+            // gossipEmitter.emit("message", createUpdateMsg(0));


Same as above.

packages/wire/lib/messages/ChannelUpdateMessage.ts

packages/wire/lib/messages/ClosingSignedMessage.ts

bmancini55

Looks awesome! Great work!

Sample Refactor 'Value'

287f1b6

Vib-UX commented Aug 5, 2021

View reviewed changes

packages/wire/__tests__/messages/ClosingSignedMessage.spec.ts Show resolved Hide resolved

Refactor BigInt --> Value

5779f61

bmancini55 suggested changes Aug 9, 2021

View reviewed changes

packages/wire/lib/messages/ChannelUpdateMessage.ts Outdated Show resolved Hide resolved

packages/wire/lib/messages/ClosingSignedMessage.ts Outdated Show resolved Hide resolved

packages/wire/lib/messages/OpenChannelMessage.ts Outdated Show resolved Hide resolved

Vib-UX added 2 commits August 10, 2021 10:15

Review V1

1fe63b9

Build Error- Trial 1

64cae65

Vib-UX requested a review from bmancini55 August 10, 2021 05:33

bmancini55 suggested changes Aug 11, 2021

View reviewed changes

packages/bitcoin/lib/Value.ts Outdated Show resolved Hide resolved

packages/bitcoin/lib/Value.ts Outdated Show resolved Hide resolved

Review V2

c8f1eb4

Vib-UX requested a review from bmancini55 August 11, 2021 23:55

Vib-UX added 2 commits August 13, 2021 03:23

@node-lightning/core

0de4062

graph/lib/error

72289df

Final-Fix-Error1

fe82b07

bmancini55 suggested changes Aug 13, 2021

View reviewed changes

Vib-UX added 2 commits August 13, 2021 21:32

Null Fix

d792690

Null Check

a13a4ef

bmancini55 approved these changes Aug 16, 2021

View reviewed changes

bmancini55 marked this pull request as ready for review August 16, 2021 15:42

bmancini55 merged commit d605c88 into node-lightning:main Aug 16, 2021

Conversation

Vib-UX commented Aug 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented Aug 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Vib-UX commented Aug 5, 2021

Uh oh!

bmancini55 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bmancini55 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bmancini55 commented Aug 11, 2021

Uh oh!

Vib-UX commented Aug 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bmancini55 commented Aug 12, 2021

Uh oh!

bmancini55 commented Aug 12, 2021

Uh oh!

Vib-UX commented Aug 12, 2021

Uh oh!

bmancini55 left a comment

Choose a reason for hiding this comment

Uh oh!

bmancini55 Aug 13, 2021

Choose a reason for hiding this comment

Uh oh!

Vib-UX Aug 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmancini55 Aug 13, 2021

Choose a reason for hiding this comment

Uh oh!

Vib-UX Aug 13, 2021

Choose a reason for hiding this comment

Uh oh!

bmancini55 Aug 13, 2021

Choose a reason for hiding this comment

Uh oh!

bmancini55 Aug 13, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bmancini55 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Vib-UX commented Aug 5, 2021 •

edited

Loading

codecov-commenter commented Aug 5, 2021 •

edited

Loading

Vib-UX commented Aug 11, 2021 •

edited

Loading

Vib-UX Aug 13, 2021 •

edited

Loading