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

fix: runner_features added in runner.py and tests modified #94

Merged
merged 1 commit into from Jul 11, 2023
Merged

fix: runner_features added in runner.py and tests modified #94

merged 1 commit into from Jul 11, 2023

Conversation

Psycho-Pirate
Copy link
Contributor

A function runner_features is added as an abstract method to the runner class. This methods provides the features required by an implementation to Lnprototest. This makes Lnprototest compatible with other implementations.
The tests are also modified to use this function in connect_to_node_helper.

@Psycho-Pirate Psycho-Pirate force-pushed the feature_function branch 2 times, most recently from e64a609 to b024075 Compare June 19, 2023 17:19
vincenzopalazzo
vincenzopalazzo reviewed Jun 19, 2023
View reviewed changes
Copy link
Collaborator

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

Some comments but overall looks good :)

lnprototest/dummyrunner.py Outdated Show resolved Hide resolved
lnprototest/runner.py Outdated Show resolved Hide resolved
lnprototest/runner.py Outdated Show resolved Hide resolved
@Psycho-Pirate
Copy link
Contributor Author

I have made the suggested changes :)

vincenzopalazzo
vincenzopalazzo requested changes Jun 20, 2023
View reviewed changes
Copy link
Collaborator

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

Ok I made another pass and I had another suggestion :)

tests/test_bolt2-01-close_channel.py Outdated Show resolved Hide resolved
vincenzopalazzo
vincenzopalazzo reviewed Jun 24, 2023
View reviewed changes
Copy link
Collaborator

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. The PR looks good. Thanks for the changes.

Can we rework the commit in two?

  1. Make the change to the lnprototest package by describing the commit and why we need it in the commit body;
  2. Make the change to the tests and describe it inside the commit body

lnprototest/runner.py Outdated Show resolved Hide resolved
@Psycho-Pirate
Copy link
Contributor Author

Sorry for the delay. The PR looks good. Thanks for the changes.

Can we rework the commit in two?

1. Make the change to the lnprototest package by describing the commit and why we need it in the commit body;

2. Make the change to the tests and describe it inside the commit body

Actually there are no changes in the tests, it just formats it when I run make fmt. The only changes are present in ln_spec_utils.py and runner.py. So should I make it in a single commit?

@vincenzopalazzo
Copy link
Collaborator

Actually there are no changes in the tests, it just formats it when I run make fmt. The only changes are present in ln_spec_utils.py and runner.py. So should I make it in a single commit?

Yeah I just noted, maybe the new black version is formatting the things in a different way! Good to go in a single block

@Psycho-Pirate
Copy link
Contributor Author

I have made the changes

vincenzopalazzo
vincenzopalazzo approved these changes Jun 25, 2023
View reviewed changes
Copy link
Collaborator

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

ACK 0dc5ddd

Thanks!

@vincenzopalazzo vincenzopalazzo added the merge blocked Currently blocked by an issue or awaiting a better design. label Jul 3, 2023
@Psycho-Pirate Psycho-Pirate reopened this Jul 3, 2023
vincenzopalazzo added a commit to vincenzopalazzo/lampo.rs that referenced this pull request Jul 11, 2023
@vincenzopalazzo
fix(lnprototest): fix some issue with the feature for the runner
42f4437 
This commit is just a hotfix we hope to merge this PR
rustyrussell/lnprototest#94 to inject
the default feature inside the runner.

Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
@vincenzopalazzo vincenzopalazzo mentioned this pull request Jul 11, 2023
Merged
@vincenzopalazzo vincenzopalazzo removed the merge blocked Currently blocked by an issue or awaiting a better design. label Jul 11, 2023
@vincenzopalazzo
Copy link
Collaborator

Ok margin this because with lnprototest and bitcoin 24 it is working fine, I think there is some problem with bitcoin 25 but this is not related to this PR

@vincenzopalazzo vincenzopalazzo merged commit 4251a32 into rustyrussell:master Jul 11, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vincenzopalazzo vincenzopalazzo vincenzopalazzo 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.

None yet
2 participants
@Psycho-Pirate @vincenzopalazzo