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

Runes: document parameter usage, and let them be ints. #6295

Merged

Conversation

rustyrussell
Copy link
Contributor

@rustyrussell rustyrussell commented May 30, 2023

(based on #6294 )

Closes: #6288

I had forgotten that we already strip punctuation from parameter names, so we merely need to treat integer parameters correctly FTW. And document that _ stripping!

@rustyrussell rustyrussell added this to the v23.08 milestone May 30, 2023
@rustyrussell rustyrussell requested a review from niftynei May 30, 2023 04:11
@rustyrussell rustyrussell requested a review from cdecker as a code owner May 30, 2023 04:11
Copy link
Contributor

@niftynei niftynei left a comment

Choose a reason for hiding this comment

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

ACK 0fa2412

@@ -427,6 +427,18 @@ static const char *check_condition(const tal_t *ctx,
if (!ptok)
return rune_alt_single_missing(ctx, alt);

/* Pass through valid integers as integers. */
if (ptok->type == JSMN_PRIMITIVE) {
Copy link
Contributor

Choose a reason for hiding this comment

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

cool this is much nicer than my suggestion./

Copy link
Contributor

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

Concept-ACK 0fa2412

Copy link
Contributor

@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 0fa2412

Nice!

This lead to confusion about how to specify `amount_msat`.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Previously any attempt would result in "is not an integer field"; we
now recognize valid JSON integers as integer fields.

Changelog-Fixed: Plugins: `commando` runes can now compare integer parameters using '<' and '>' as expected.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
It sometimes takes > 1800 seconds under valgrind.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell
Copy link
Contributor Author

Marked command stresstest as slow, to prevent timeouts.

Ack b1e2786

@rustyrussell rustyrussell force-pushed the guilt/runes-underscores branch from b1e2786 to b29732e Compare June 6, 2023 07:04
Copy link
Contributor

@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 b29732e

@rustyrussell rustyrussell merged commit 55119e6 into ElementsProject:master Jun 6, 2023
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.

3 participants