Skip to content

Conversation

@chedieck
Copy link
Collaborator

@chedieck chedieck commented Jan 18, 2024

Related to #774

Depends on

Description

Parses the opReturn string conforming to the specs defined in https://reviews.bitcoinabc.org/D15176, and
adds the variable to the paybutton triggers.

Test plan

As discussed in PayButton/paybutton#302; I've added a basic support for JSON and arrays.

If a PayButton, created using the branch in the linked dependency, is created with op-return="key=value separated=by spaces=and arrays=are|done|this|way"
This will should result in:

{
  paymentId: '66c682719fb0e45e',
  data: {
    key: 'value',
    separated: 'by',
    spaces: 'and',
    arrays: ['are', 'done', 'this', 'way']
  }
}

Something not parsable such as op-return="mySimpleText with an equal sign at the end to add confusion=" should then return:

{
  paymentId: '66c682719fb0e45e',
  data: "mySimpleText with an equal sign at the end to add confusion="
}

In other words:
When using this PayButton to make a payment, this is what should be saved to the database and also what should be POST requested by the trigger in place of the <opReturn> variable.

Remarks

  • To see what is relevant in the DB, one could do SELECT hash, amount, opReturn from Transaction where not opReturn='';

  • Make sure also there isn't edge cases not covered by the unittests and that the results there established are indeed the expected ones.

@chedieck chedieck changed the title Feat/op return on triggers [#774] feat: op return on triggers Jan 18, 2024
@chedieck chedieck changed the title [#774] feat: op return on triggers [#774] feat: op return new specs Jan 18, 2024
@chedieck chedieck marked this pull request as draft January 18, 2024 20:02
@chedieck chedieck marked this pull request as ready for review January 18, 2024 20:43
@Klakurka Klakurka self-requested a review January 19, 2024 00:03
Copy link
Member

@Klakurka Klakurka left a comment

Choose a reason for hiding this comment

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

Really nice!

Just to double check that it's as expected:

Sent: classOf=2013 bullYears=2013|2017|20216034
Saved: '4dd801996dab0db21d38bbfdc17388edb02ba739929beaa12e46b47eb8d76d7a', '100.00000000', '{\"data\":{\"classOf\":\"2013\",\"bullYears\":[\"2013\",\"2017\",\"20216034\"]},\"paymentId\":\"7cdc8ff82a484b77\"}'

Sent: classOf=2013 bullYears=2013|2017|2021 4666
Saved: 'f5aae24116bf0d19ab0349169decc6c70937f5616d42fe6c9684d4d8945dea5b', '100.00000000', '{\"data\":[\"classOf=2013 bullYears=2013\",\"2017\",\"2021 4666\"],\"paymentId\":\"3a227fda2001a645\"}'

The key-less value at the end in the 2nd one causes it to "fail" (to parse it).

Any options for allowing a bit of text w/ spaces in it (eg. using single quotes)? Currently, it doesn't look like it's supported but it would be a nice thing to allow.

@chedieck
Copy link
Collaborator Author

Any options for allowing a bit of text w/ spaces in it (eg. using single quotes)? Currently, it doesn't look like it's supported but it would be a nice thing to allow.

The problem with mixing the text with key=value (e.g. invoice=312 foo is that when generating the data object it is clear that data: { invoice: '312' } but it is not clear how foo should appear. We could create a text field like { invoice: '312', text: 'foo'} but then it is not clear what should happen if e.g. invoice=312 foo bar, invoice=312 text=foo bar or invoice=312 bar text=foo

I decided to go with the approach of 'parse all or nothing' not only for those reasons but also because then it's clear when it has gone wrong. E.g. if the user wanted to do invoice=312 foo=bar but missed the second equal sign: invoice=312 foobar; he will get just text and see it has gone wrong; but if he gets something like { invoice: '312', text: 'foobar'} it's possible that his code goes right up to a certain point (parsing the invoice) and later fails when it doesn't find 'foo'

It's also more predictable and easier for the user to understand this more simple logic than a complex parsing that tries many things until it gives up on parsing.

@Klakurka Klakurka merged commit 2a52d1d into master Jan 19, 2024
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