Tweak checkrune API (nodeid isn't required) and update docs. #6622
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ShahanaFarooqui
force-pushed
the
guilt/rune-details-docs
branch
from
ed4e709
to
0fb8984
Compare
rustyrussell
force-pushed
the
guilt/rune-details-docs
branch
from
0fb8984
to
4d82620
Compare
ShahanaFarooqui
force-pushed
the
guilt/rune-details-docs
branch
from
4d82620
to
75f4345
Compare
nepet
reviewed
Sep 10, 2023
Comment on lines
676
to
+725
| } else if (streq(alt->fieldname, "id")) { | ||
| const char *id = node_id_to_hexstr(tmpctx, cinfo->peer); | ||
| return rune_alt_single_str(ctx, alt, id, strlen(id)); | ||
| if (cinfo->peer) { | ||
| const char *id = node_id_to_hexstr(tmpctx, cinfo->peer); | ||
| return rune_alt_single_str(ctx, alt, id, strlen(id)); | ||
| } | ||
| return rune_alt_single_missing(ctx, alt); |
There was a problem hiding this comment.
Let me check if I understand these lines correctly:
- We have an alternative with with a fieldname
id - We check if we got a (now optional)
nodeid, that should reside incinfo->peer - If so, check the id against the alternatives rule.
- If not, check if the alternative is of "missing field" type (or comment).
If this is correct, this seems likely the correct way to treat a field set of<id> requested <method> with <params> at <time> with request <rate>.
This however makes the "optional" arguments for checkrune not as "optional" as it might seem. We may consider adding 1 or 2 sentences to the docs that explain the behavior and the implications that an "unset" argument has.
There was a problem hiding this comment.
? Every optional field (e.g. parameter names) is the same. They fail any test (comparison, equality, etc), except the "not present" test.
This cannot break any existing usage, since that always sets id. I can't think of what I would document here?
rustyrussell
force-pushed
the
guilt/rune-details-docs
branch
from
75f4345
to
fb40b7c
Compare
There's actually no "filter" here, this was mistakenly taken from commando. Signed-off-by: Rusty Russell <[email protected]>
rustyrussell
force-pushed
the
guilt/rune-details-docs
branch
from
fb40b7c
to
e64fe60
Compare
Usage line isn't correct, as fields are not optional, and return needs fleshing out for error codes. Signed-off-by: Rusty Russell <[email protected]>
Signed-off-by: Rusty Russell <[email protected]>
nodeid is only useful when we know the peer we're talking to (e.g. commando). Signed-off-by: Rusty Russell <[email protected]> No-schema-diff-check: We're simply making optional, not deprecating!
These look like uniqueids, and so can confuse us (I discovered this by making a typo in a test!) Signed-off-by: Rusty Russell <[email protected]>
This was a misunderstanding: nodeid is useful for commando, where it's the peer's nodeid, and Noise-XK guarantees that we know who that is. It's not useful for clnrest, so don't require it (it was our node id, which is redundant). Signed-off-by: Rusty Russell <[email protected]>
It always is for runes we create, but in theory you can take our secret key and make our own runes with your own tools. (We correctly refuse runes without uniqueids if they're *not* ours anyway: uniqueid is only used for our own runes). Signed-off-by: Rusty Russell <[email protected]>
This seems to be a cut & paste bug (mine, AFAICT!) from the command code: ``` rune = rune_derive_start(cmd, master_rune, tal_fmt(tmpctx, "%"PRIu64, rune_counter ? *rune_counter : 0)); ``` In that case, rune_counter was a pointer, which could be NULL. Signed-off-by: Rusty Russell <[email protected]>
rustyrussell
force-pushed
the
guilt/rune-details-docs
branch
from
e64fe60
to
40fe0b4
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Builds on #6617(Rebased on master)Also tighten our rune checking on our own runes which were generated from other code (a theoretical possibility), and make sure we only add valid restrictions...