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

Offers deprecate string recurrence #7034

Conversation

rustyrussell
Copy link
Contributor

@cdecker points out that it's ugly to have a string variant, and I agree. Deprecate now (quickly, it's experimental) so GRPC/Rust bindings don't have to support it!

@rustyrussell rustyrussell added this to the v24.02 milestone Feb 3, 2024
@rustyrussell rustyrussell changed the title Ooffers deprecate string recurrence Offers deprecate string recurrence Feb 3, 2024
It can actually be useful for more complex parameter parsing, as we're about to see.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Christian points out that this makes the type harder, and it's just awkward.

Changelog-EXPERIMENTAL: JSON-RPC: Deprecated `offer` parameter `recurrence_base` with `@` prefix: use `recurrence_start_any_period`.
Changelog-EXPERIMENTAL: JSON-RPC: Added `offer` parameter `recurrence_start_any_period`.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell rustyrussell force-pushed the guilt/offers-deprecate-string-recurrence branch from 4533e45 to 9d7055d Compare February 3, 2024 23:52
@cdecker
Copy link
Member

cdecker commented Feb 4, 2024

This is missing the update to the docs/schema/offer.schema.json which is where the issue arises in the first place when trying to get it under msggen coverage. I'll add that and then merge.

In the meantime, thank you very much for this @rustyrussell ^^

@cdecker
Copy link
Member

cdecker commented Feb 4, 2024

Turns out this doesn't address the schema issues we have:

FAILED tests/test_pay.py::test_fetchinvoice_recurrence - jsonschema.exceptions.ValidationError: '1minutes' is not of type 'object'

Failed validating 'type' in schema['properties']['recurrence']:
    {'properties': {'period': {'description': 'the positive number which '
                                              'the time unit refers to',
                               'type': 'u32'},
                    'time_unit': {'description': 'a positive number '
                                                 'referring to a time unit '
                                                 '- 0 (seconds), 1 (days), '
                                                 '2 (months), 3 (years)',
                                  'type': 'u32'},
                    'time_unit_name': {'description': 'the name of the '
                                                      'specified time unit '
                                                      '- either  '
                                                      '"seconds", '
                                                      '"minutes", "hours", '
                                                      '"days", "weeks" or '
                                                      '"years"',
                                       'type': 'string'}},
     'required': ['time_unit', 'period'],
     'type': 'object'}

On instance['recurrence']:
    '1minutes'

This error is caused in #6896 after applying this patch. We need to remove the shorthand usage of 1minutes in the tests in order to fix this.

@cdecker
Copy link
Member

cdecker commented Feb 4, 2024

Nevertheless, this is a good change, let's merge it asap, and then address that request schema issue, using redundant argument encodings.

@rustyrussell
Copy link
Contributor Author

That's different. That json schema is wrong: recurrence is a string, never a raw number.

If we wanted, we could make it an object, in which case we would use a number and a string (an enum in JSON terms). But using the draft bolt encoding here is wrong, since it's highly experimental and subject to change.

TBH, I would not support recurrence in the GRPC API for now.

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 9d7055d

The CI isssue was already reported as a issue, and looks like not related to this PR #7043

@rustyrussell rustyrussell merged commit f56b9e9 into ElementsProject:master Feb 6, 2024
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants