Skip to content
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.

Sign up for GitHub

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

Jump to bottom

2829 review club #29

Merged
merged 1 commit into from
Feb 21, 2024
Merged

2829 review club #29

merged 1 commit into from
Feb 21, 2024

Conversation

jbesraa
Copy link
Contributor

@jbesraa jbesraa commented Feb 12, 2024

No description provided.

@netlify Netlify
Copy link

netlify bot commented Feb 12, 2024 •
edited
Loading

✅ Deploy Preview for courageous-frangipane-fab648 ready!

Name Link
🔨 Latest commit 7a6344a
🔍 Latest deploy log https://app.netlify.com/sites/courageous-frangipane-fab648/deploys/65d6316bc86f200008667212
😎 Deploy Preview https://deploy-preview-29--courageous-frangipane-fab648.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@jbesraa jbesraa force-pushed the add-2829-part-1 branch 2 times, most recently from 56bc703 to 2d428f8 Compare February 14, 2024 09:48
@jbesraa jbesraa marked this pull request as ready for review February 14, 2024 09:48
@dunxen
Copy link
Collaborator

dunxen commented Feb 16, 2024

Nice. I like the addition of the bLIP review. I do think we could expand on the questions to cover more detail and specifics, though. I'll review and give them some thought. If you have any additional specific questions in mind then it would be great to add some!

We could dive deeper into the implementation itself.

@jbesraa
Copy link
Contributor Author

jbesraa commented Feb 16, 2024

Nice. I like the addition of the bLIP review. I do think we could expand on the questions to cover more detail and specifics, though. I'll review and give them some thought. If you have any additional specific questions in mind then it would be great to add some!

We could dive deeper into the implementation itself.

Added another question, and I have a couple of follow up questions if the audience will be engaging enough.
Notice that this sessions is a first of two and in the second one we will dive into the pr

@dunxen
Copy link
Collaborator

dunxen commented Feb 19, 2024

Added another question, and I have a couple of follow up questions if the audience will be engaging enough.

Notice that this sessions is a first of two and in the second one we will dive into the pr

Ok great! Just a few review comments then I'll merge today!

jkczyz
jkczyz reviewed Feb 21, 2024
View reviewed changes
_posts/2024-02-16-#2829.md Outdated
@@ -0,0 +1,31 @@
---
layout: pr
date: 2024-02-16
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this needs to be update.

_posts/2024-02-16-#2829.md Outdated
## Questions
1. Did you review bLIP31? [Concept ACK, approach ACK, tested ACK, or NACK](https://github.com/lightningdevkit/rust-lightning/blob/master/CONTRIBUTING.md#peer-review)?
2. What are the capabilities [bLIP/31] introduce?
3. Who initiae the messaging process? what are the pros/cons?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/initiae/initiates

_posts/2024-02-16-#2829.md Outdated

## Questions
1. Did you review bLIP31? [Concept ACK, approach ACK, tested ACK, or NACK](https://github.com/lightningdevkit/rust-lightning/blob/master/CONTRIBUTING.md#peer-review)?
2. What are the capabilities [bLIP/31] introduce?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/introduce/introduces

_posts/2024-02-16-#2829.md Outdated
Comment on lines 20 to 22
1. Did you review bLIP31? [Concept ACK, approach ACK, tested ACK, or NACK](https://github.com/lightningdevkit/rust-lightning/blob/master/CONTRIBUTING.md#peer-review)?
2. What are the capabilities [bLIP/31] introduce?
3. Who initiae the messaging process? what are the pros/cons?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/what/What

_posts/2024-02-16-#2829.md Outdated
2. What are the capabilities [bLIP/31] introduce?
3. Who initiae the messaging process? what are the pros/cons?
4. How does [bLIP/31] relate to [bolt/11] and [bolt/12]?
5. How the message sender know they are able to exchange a message with the recipient?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/How/How does

@jbesraa
Copy link
Contributor Author

jbesraa commented Feb 21, 2024

@jkczyz thanks!
addressed all the points

@dunxen
Copy link
Collaborator

dunxen commented Feb 21, 2024

Oof my reviews were still "pending" as I forgot to hit submit but @jkczyz got all of them 🤦‍♂️

@dunxen dunxen merged commit a5a541a into lightningdevkit:main Feb 21, 2024
4 checks passed
@jbesraa
Copy link
Contributor Author

jbesraa commented Feb 21, 2024

Ah no worries! Thanks for merging. In the next few days ill open part 2 of this.

In the next couple of sessions after that, gonna cover the new dns bLIP.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jkczyz jkczyz jkczyz left review comments

@dunxen dunxen dunxen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jbesraa @dunxen @jkczyz