-
Notifications
You must be signed in to change notification settings - Fork 586
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
test: add UpdateConnection to endpoint and update usages in tests. #5719
test: add UpdateConnection to endpoint and update usages in tests. #5719
Conversation
connection := endpoint.GetConnection() | ||
updater(&connection) | ||
|
||
endpoint.SetConnection(connection) |
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.
don't need to commit and update client as original issue had. Lmk if it would be good to add for some reason.
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.
It's only necessary if we need to generate a proof on the modified connection
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.
you think its beneficial to slip it back in? don't currently do that atm but maybe in futureland?
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 think it could be done manually when required
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.
LGTM, I really like how much code can be refactored to use the new fn ❤️
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.
A long awaited feature! 😃 I love the improvement!! Are there any extra functions we should deprecate/remove now?
connection := endpoint.GetConnection() | ||
updater(&connection) | ||
|
||
endpoint.SetConnection(connection) |
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.
It's only necessary if we need to generate a proof on the modified connection
Not for connection case thankfully! Only for channel updating func! |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5719 +/- ##
==========================================
- Coverage 81.54% 81.50% -0.04%
==========================================
Files 199 199
Lines 15225 15225
==========================================
- Hits 12415 12409 -6
- Misses 2344 2349 +5
- Partials 466 467 +1 |
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.
LGTM! Nice work @DimitrisJim :)
connection := endpoint.GetConnection() | ||
updater(&connection) | ||
|
||
endpoint.SetConnection(connection) |
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 think it could be done manually when required
Description
closes: #3985
Commit Message / Changelog Entry
see the guidelines for commit messages. (view raw markdown for examples)
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/
) or specification (x/<module>/spec/
).godoc
comments.Files changed
in the Github PR explorer.Codecov Report
in the comment section below once CI passes.