Skip to content

Updates to PaymentRequest stuctures to align with the latest TSC spec.#69

Closed
AustEcon wants to merge 8 commits intomasterfrom
features/updates-for-tsc-spec-compliance
Closed

Updates to PaymentRequest stuctures to align with the latest TSC spec.#69
AustEcon wants to merge 8 commits intomasterfrom
features/updates-for-tsc-spec-compliance

Conversation

@AustEcon
Copy link
Collaborator

@AustEcon AustEcon commented Jul 28, 2022 •
edited
Loading

As discussed with @jadwahab and @sirdeggen here is the PR for updating to the latest TSC spec: https://tsc.bitcoinassociation.net/standards/direct_payment_protocol/

Corresponding dual PR here: bitcoin-sv/dpp-proxy#57

So far this only updates the PaymentRequest structure which is enough for now to get ElectrumSV through the first part of the data flow for development...

Still to do:

  • Payment
  • PaymentACK
  • Adequate testing end-to-end against this code

Until the dpp-proxy is updated to the TSC spec, ElectrumSV will be building against this feature branch to avoid technical debt of the old message structures. So if you are not a fan of this implementation, that is no problem - it still unblocks Roger and I to keep progressing the ElectrumSV integration until the dpp-proxy master branch catches up. 😃

github-actions[bot]
github-actions bot reviewed Jul 28, 2022
View reviewed changes
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Congrats, you just opened your first pull request on libsv/go-payment_protocol! Thank you for contributing!

@mergify mergify bot added the feature label Jul 28, 2022
@AustEcon AustEcon mentioned this pull request Jul 28, 2022
Open
3 tasks
@AustEcon AustEcon force-pushed the features/updates-for-tsc-spec-compliance branch from c75950b to 0d29560 Compare July 28, 2022 09:51
jadwahab
jadwahab reviewed Jul 28, 2022
View reviewed changes
@AustEcon AustEcon force-pushed the features/updates-for-tsc-spec-compliance branch 2 times, most recently from 78b9d92 to 52bbdfb Compare July 29, 2022 13:51
@AustEcon
Updates to Payment and PaymentACK message structures for TSC comp…
eb06553 
…liance

- Rename `PaymentRequest` -> `PaymentTerms` as `PaymentRequest` was the old BIP270 terminology for this struct
- Update the payment.go:Validate() method for the new `Payment` struct
@AustEcon AustEcon force-pushed the features/updates-for-tsc-spec-compliance branch from 52bbdfb to eb06553 Compare July 29, 2022 13:53
jadwahab
jadwahab reviewed Jul 30, 2022
View reviewed changes
payment_request.go Outdated
@@ -57,5 +104,5 @@ type PaymentRequestService interface {

// PaymentRequestReader will return a new payment request.
type PaymentRequestReader interface {
Copy link
Member

@jadwahab jadwahab Jul 30, 2022

Choose a reason for hiding this comment

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

shouldn't all places where we have PaymentRequest be PaymentTerms instead?

Copy link
Collaborator Author

@AustEcon AustEcon Aug 1, 2022

Choose a reason for hiding this comment

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

Yes. I suppose it should. I have now updated all instances of PaymentRequest to PaymentTerms for go-dpp and the dpp-proxy.

This includes the websocket message types: paymentterms.create, paymentterms.response and paymentterms.error

For consistency.

jadwahab
jadwahab requested changes Jul 30, 2022
View reviewed changes
Copy link
Member

@jadwahab jadwahab left a comment

Choose a reason for hiding this comment

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

I think we should have different folders/modules for different payment modes, what do you think? would be worth asking @theflyingcodr

@AustEcon AustEcon force-pushed the features/updates-for-tsc-spec-compliance branch 3 times, most recently from 87877c1 to adee145 Compare August 1, 2022 08:38
@AustEcon
Copy link
Collaborator Author

AustEcon commented Aug 1, 2022 •
edited
Loading

I think we should have different folders/modules for different payment modes, what do you think? would be worth asking @theflyingcodr

Seems like a good suggestion.

I have refactored into a modes package with nested hybridmode. LMK if that's kind of what you're thinking.

@AustEcon AustEcon requested a review from jadwahab August 1, 2022 10:08
@AustEcon AustEcon force-pushed the features/updates-for-tsc-spec-compliance branch 3 times, most recently from 33a1dbc to 94bb7bf Compare August 1, 2022 10:32
@AustEcon
Rename all instances of PaymentRequest to PaymentTerms & refactor…
246f1c1 
…ing to place each of the three dpp message type structs in their own folders
@AustEcon AustEcon force-pushed the features/updates-for-tsc-spec-compliance branch from 94bb7bf to 246f1c1 Compare August 1, 2022 10:48
@mergify
Copy link

mergify bot commented Aug 29, 2022

This pull request looks stale. Feel free to reopen it if you think it's a mistake.

@mergify mergify bot closed this Aug 29, 2022
@mergify mergify bot added the stale label Aug 29, 2022
@sirdeggen
Copy link

sirdeggen commented Sep 1, 2022

Still discussing, sorry @AustEcon - early next week should have resolved the differences in opinion on this.

@sirdeggen sirdeggen reopened this Sep 1, 2022
@rt121212121
Update go-dpp for dpp-proxy to expect the secure by default `JSON…
1b1f7e6 
…Envelope` wrapped `PaymentTerms` as the standard.
@rt121212121 rt121212121 force-pushed the features/updates-for-tsc-spec-compliance branch 4 times, most recently from 1d3f2db to c0a49ee Compare September 17, 2022 00:10
@rt121212121 rt121212121 force-pushed the features/updates-for-tsc-spec-compliance branch from c0a49ee to 330cf50 Compare September 17, 2022 00:15
@mergify
Copy link

mergify bot commented Oct 8, 2022

This pull request looks stale. Feel free to reopen it if you think it's a mistake.

@mergify mergify bot closed this Oct 8, 2022
@AustEcon
Remove Memo and Error fields of the PaymentACK struct because t…
b674402 
…hese fields do not feature in the TSC specification
@AustEcon AustEcon reopened this Oct 11, 2022
@mergify
Copy link

mergify bot commented Nov 1, 2022

This pull request looks stale. Feel free to reopen it if you think it's a mistake.

@mergify mergify bot closed this Nov 1, 2022
@AustEcon AustEcon reopened this Nov 1, 2022
@mergify
Copy link

mergify bot commented Nov 22, 2022

This pull request looks stale. Feel free to reopen it if you think it's a mistake.

@mergify mergify bot closed this Nov 22, 2022
@AustEcon AustEcon reopened this Sep 28, 2023
@mergify mergify bot closed this Oct 19, 2023
@mergify
Copy link

mergify bot commented Oct 19, 2023

This pull request looks stale. Feel free to reopen it if you think it's a mistake.

This was referenced Nov 1, 2023
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

@jadwahab jadwahab Awaiting requested review from jadwahab

Assignees

@AustEcon AustEcon

Labels

feature stale

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@AustEcon @sirdeggen @jadwahab @rt121212121