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

addpsbtoutput: New onchain command for PSBT output and splice_out tests #6676

Merged
merged 1 commit into from
Oct 3, 2023

Conversation

ddustin
Copy link
Collaborator

@ddustin ddustin commented Sep 13, 2023

Note: This now depends on PR #6677

New addpsbtoutput command for creating a PSBT that can receive funds to the on-chain wallet.

@ddustin ddustin added this to the v23.08 Point Releases milestone Sep 13, 2023
@ddustin ddustin force-pushed the ddustin/recvpsbt branch 4 times, most recently from 1910afc to 94c7e83 Compare September 14, 2023 00:06
doc/schemas/recvpsbt.schema.json Outdated Show resolved Hide resolved
contrib/pyln-client/pyln/client/lightning.py Outdated Show resolved Hide resolved
tests/test_splicing.py Outdated Show resolved Hide resolved
@ddustin ddustin force-pushed the ddustin/recvpsbt branch 4 times, most recently from dbec2b6 to 524de4a Compare September 14, 2023 20:42
@ddustin
Copy link
Collaborator Author

ddustin commented Sep 19, 2023

You need to write a separate set of tests for recvpsbt (is that a good name? addpsbtoutput?). It must test the case where we create, and simply enhance, a PSBT.

I'm leaning toward newoutput to stay in theme with the existing newaddr. How does that sound?

Updated this PR with newoutput and other fixes -- it should be ready to go once when it passes CI.

@ddustin ddustin force-pushed the ddustin/recvpsbt branch 2 times, most recently from 1a6f5c2 to ce2a618 Compare September 19, 2023 18:10
Copy link
Contributor

@rustyrussell rustyrussell left a comment

Choose a reason for hiding this comment

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

Just bikeshedding the name now...

@@ -1523,6 +1523,17 @@ def fundpsbt(self, satoshi, feerate, startweight, minconf=None, reserve=None, lo
}
return self.call("fundpsbt", payload)

def newoutput(self, satoshi, initialpsbt=None, locktime=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

Really needs a psbt in here somewhere!

psbtaddoutput?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, commit name is now wrong!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Whoops I read that as addpsbtoutput and changed the name to that 🤦. I kinda like it though -- what do you think?

doc/schemas/newoutput.request.json Outdated Show resolved Hide resolved
@ddustin ddustin force-pushed the ddustin/recvpsbt branch 2 times, most recently from db6d23e to eb156b4 Compare September 21, 2023 02:31
@ddustin ddustin changed the title recvpsbt: New onchain command for PSBT output addpsbtoutput: New onchain command for PSBT output Sep 21, 2023
@ddustin ddustin changed the title addpsbtoutput: New onchain command for PSBT output addpsbtoutput: New onchain command for PSBT output and splice_out tests Sep 21, 2023
@rustyrussell
Copy link
Contributor

Looks like you returned success when you meant to return an error:

FAILED tests/test_splicing.py::test_invalid_splice - jsonschema.exceptions.ValidationError: Additional properties are not allowed ('error', 'message' were unexpected)

Failed validating 'additionalProperties' in schema:
    {'$schema': 'http://json-schema.org/draft-07/schema#',
     'added': 'v23.08',
     'additionalProperties': False,
     'properties': {'commitments_secured': {'description': 'whether or not '
                                                           'the '
                                                           'commitments '
                                                           'were secured',
                                            'type': 'boolean'},
                    'psbt': {'description': 'the (incomplete) PSBT of the '
                                            'splice transaction',
                             'type': 'string'}},
     'required': ['psbt', 'commitments_secured'],
     'type': 'object'}

On instance:
    {'error': 'You provided 1000000000msat but committed to '
              '1100000000msat.',
     'message': 'Splice funding too low'}

@ddustin
Copy link
Collaborator Author

ddustin commented Sep 21, 2023

Looks like you returned success when you meant to return an error:

FAILED tests/test_splicing.py::test_invalid_splice - jsonschema.exceptions.ValidationError: Additional properties are not allowed ('error', 'message' were unexpected)

Failed validating 'additionalProperties' in schema:
    {'$schema': 'http://json-schema.org/draft-07/schema#',
     'added': 'v23.08',
     'additionalProperties': False,
     'properties': {'commitments_secured': {'description': 'whether or not '
                                                           'the '
                                                           'commitments '
                                                           'were secured',
                                            'type': 'boolean'},
                    'psbt': {'description': 'the (incomplete) PSBT of the '
                                            'splice transaction',
                             'type': 'string'}},
     'required': ['psbt', 'commitments_secured'],
     'type': 'object'}

On instance:
    {'error': 'You provided 1000000000msat but committed to '
              '1100000000msat.',
     'message': 'Splice funding too low'}

Whoops, should be fixed now

Copy link
Contributor

@rustyrussell rustyrussell left a comment

Choose a reason for hiding this comment

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

One v. minor change: the version number it was added in...

doc/schemas/addpsbtoutput.schema.json Outdated Show resolved Hide resolved
@ddustin ddustin force-pushed the ddustin/recvpsbt branch 2 times, most recently from 9986a7e to 68e3445 Compare September 27, 2023 14:56
@rustyrussell
Copy link
Contributor

rustyrussell commented Oct 2, 2023

Trivial rebase on master for CI...

Ack 10e1224

Also added splice_out tests that use the new PSBT command.

ChangeLog-Added: New `addpsbtoutput` command for creating a PSBT that can receive funds to the on-chain wallet.
Comment on lines +685 to +686
}
else if(locktime) {
Copy link
Collaborator

@vincenzopalazzo vincenzopalazzo Oct 3, 2023

Choose a reason for hiding this comment

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

Suggested change
}
else if(locktime) {
} else if (locktime) {

FYI just a minor nit

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 f1dfec4

@vincenzopalazzo vincenzopalazzo merged commit bc9333a into ElementsProject:master Oct 3, 2023
34 of 38 checks passed
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.

4 participants