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

runes: fix checkrune when method parameter is the empty string. #6759

Merged
merged 2 commits into from
Oct 12, 2023
Merged

runes: fix checkrune when method parameter is the empty string. #6759

merged 2 commits into from
Oct 12, 2023

Conversation

tonyaldon
Copy link
Contributor

This fix #6725.

vincenzopalazzo

This comment was marked as off-topic.

Sign in to view

@vincenzopalazzo vincenzopalazzo added this to the v23.11 milestone Oct 10, 2023
@tonyaldon tonyaldon force-pushed the checkrune-issue-6725 branch from b0de248 to a708ae1 Compare October 10, 2023 18:03
@tonyaldon
Copy link
Contributor Author

I am thinking about why this is not NULL, I should check the p_opt. If the value is not specified I expect that the value will be NULL and not empty

The value is specified, we pass the empty string "" as method parameter and this is the problem of the issue #6725.

You can check the test test_rune_method_not_equal_and_method_empty added in that PR.

p_opt is working as expected.

@vincenzopalazzo
Copy link
Collaborator

The value is specified, we pass the empty string "" as method parameter and this is the problem of the issue #6725.

  • Vincent should read the issue :)

vincenzopalazzo
vincenzopalazzo requested changes Oct 10, 2023
View reviewed changes
tests/test_runes.py Outdated Show resolved Hide resolved
@tonyaldon
pytest: test checkrune when method parameter is the empty string.
5641c5d 
Add test `test_rune_method_not_equal_and_method_empty` that reproduces
issue ElementsProject#6725.

This fails currently, so next commit fix it up.

Error:

```
>       with pytest.raises(RpcError, match='Not permitted: method not present'):
E       Failed: DID NOT RAISE <class 'pyln.client.lightning.RpcError'>

tests/test_runes.py:605: Failed
```
@tonyaldon
runes: fix checkrune when method parameter is the empty string.
f0f9bb2 
This fix ElementsProject#6725.

Changelog-Fixed: fix `checkrune` when `method` parameter is the empty string.
@tonyaldon tonyaldon force-pushed the checkrune-issue-6725 branch from a708ae1 to f0f9bb2 Compare October 10, 2023 20:48
@ShahanaFarooqui ShahanaFarooqui self-requested a review October 11, 2023 01:51
@ShahanaFarooqui
Copy link
Collaborator

@tonyaldon Great catch. Thanks for the fix.

ACK f0f9bb2.

vincenzopalazzo
vincenzopalazzo approved these changes Oct 12, 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 f0f9bb2

@vincenzopalazzo vincenzopalazzo merged commit ae94be4 into ElementsProject:master Oct 12, 2023
32 of 38 checks passed
@rustyrussell rustyrussell mentioned this pull request Oct 16, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ShahanaFarooqui ShahanaFarooqui ShahanaFarooqui approved these changes

@vincenzopalazzo vincenzopalazzo vincenzopalazzo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v23.11
Development

Successfully merging this pull request may close these issues.

checkrune: when we pass an empty value for method parameter, it seems that checkrune return {"valid": true}
3 participants
@tonyaldon @vincenzopalazzo @ShahanaFarooqui