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.

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

Revert "Use sender instead of providing actor refs (#1379)" #1391

Merged
merged 4 commits into from
Apr 24, 2020

Conversation

pm47
Copy link
Member

@pm47 pm47 commented Apr 24, 2020

It was error-prone (cf bff833b) and not a good practice according to @sstone.

Commit 2ad91b2 is optional, let me know what you think.

@pm47 pm47 requested a review from t-bast April 24, 2020 13:43
@t-bast
Copy link
Member

t-bast commented Apr 24, 2020

I like the revert, but not sure about 2ad91b2
While it simplifies a bit the code, PeerRoutingMessages are not necessarily gossip, so if we want to make those evolve independently (add new fields) we'll likely need to revert that.
Not a strong NACK, but not a strong ACK either :)

@pm47
Copy link
Member Author

pm47 commented Apr 24, 2020

I like the revert, but not sure about 2ad91b2
While it simplifies a bit the code, PeerRoutingMessages are not necessarily gossip, so if we want to make those evolve independently (add new fields) we'll likely need to revert that.
Not a strong NACK, but not a strong ACK either :)

I reverted with 13ea541 (insert 'yo_dawg.jpg') but kept the LightningMessage->RoutingMessage signature change.

@pm47 pm47 merged commit 9c748e4 into master Apr 24, 2020
@pm47 pm47 deleted the revert-use-sender branch April 24, 2020 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants