Page MenuHomePhabricator

[ecash-script] Add option to return pushdata to existing consumeNextPush function
ClosedPublic

Authored by bytesofman on Jun 7 2023, 19:01.

Details

Summary

Add optional param returnPushdata which will cause consumeNextPush to return {push, pushdata} instead of push

Test Plan

npm test

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Fabien requested changes to this revision.Jun 8 2023, 13:22
Fabien added a subscriber: Fabien.

That's a bad API. As a general rule, don't have a function do 2 different things depending on a flag but create 2 functions instead.
Here you have 2 solutions:

  • Create a new function that calls consumeNextPush, and returns the data in addition
  • Just return the pushed data unconditionally and the callsite can drop the push if unused.
This revision now requires changes to proceed.Jun 8 2023, 13:22

Change consumeNextPush to always return an object instead of a string, change version number to reflect breaking change

Fabien requested changes to this revision.Jun 8 2023, 18:03
Fabien added inline comments.
modules/ecash-script/README.md
26 ↗(On Diff #40666)

that's quite the opposite, no ? The push operator is 04 and the pushed data is 2e786563 (pushop is also more explicit than push imo)

modules/ecash-script/src/script.js
109 ↗(On Diff #40666)
if (firstByte === opReturn.OP_PUSHDATA1) {
        // Save OP code
        pushdataOp = opReturn.OP_PUSHDATA1;
if (firstByte === opReturn.OP_PUSHDATA2) {
        // The next two bytes contain the number of bytes to be pushed onto the stack in little endian order.

        // Save OP code
        pushdataOp = opReturn.OP_PUSHDATA2;
if (firstByte === opReturn.OP_PUSHDATA4) {
        // The next four bytes contain the number of bytes to be pushed onto the stack in little endian order.

        // Save OP code
        pushdataOp = opReturn.OP_PUSHDATA4;

Please read your code. This kind of absurd pattern should not show up during diff review. Just looking at your code should make it obvious that this needs to be refactored.

125 ↗(On Diff #40666)

This is backwards push is the actual data

This revision now requires changes to proceed.Jun 8 2023, 18:03
bytesofman marked 3 inline comments as done.

less ambiguous variables, cleaning up variable assignment in response to feedback

Fabien requested changes to this revision.Jun 9 2023, 17:55
Fabien added inline comments.
modules/ecash-script/src/script.js
72 ↗(On Diff #40704)

as per the following code, pushOpCode would be a better name than firstByte

77 ↗(On Diff #40704)

imo you should return the opcode in pushedWith. The value didn't appear on stack on its own

81 ↗(On Diff #40704)

you already have pushedBytecountHex, you don't need pushedBytecountDecimal. Just set it once before use with no intermediate variable. If you think about it, the endianness is not an issue.

90 ↗(On Diff #40704)

Why is that a special case and not the standard workflow ?

This revision now requires changes to proceed.Jun 9 2023, 17:55
bytesofman marked an inline comment as done.

simplifying, improved variable names

modules/ecash-script/src/script.js
77 ↗(On Diff #40704)

in this case, consumeNextPush({remainingHex: '00'}) would resolve to

{data: '00', pushedWith: '00'}

The value did appear on stack on its own. Nothing else is on the stack except for 00.

Either way, these one-byte pushes present an exceptional circumstance.

1 - They have pushedWith data that is the same as data
2 - They are the only possible data results with '' as pushedWith

... I'll return the op code since yeah, it is pushedWith 00 in this case.

90 ↗(On Diff #40704)

modified so this is standard. there is still a slight exception to handle for this case

Fabien requested changes to this revision.Jun 10 2023, 07:26

One more comment and it should be good to go

modules/ecash-script/src/script.js
101 ↗(On Diff #40706)

Wow, look at how much cleaner this code is now!

110 ↗(On Diff #40706)

Nit: technically you have a push opcode but no pushed byte count, so it should be the opposite:

pushedWith: `${pushOpCode}${pushOpCode !== pushBytecountHex ? pushBytecountHex : ''}`

In any case please add a comment to explain why this is needed, as it may trigger you in the distant future.

This revision now requires changes to proceed.Jun 10 2023, 07:26

respond to feedback, add explanation of conditional return

Fabien added inline comments.
modules/ecash-script/src/script.js
106 ↗(On Diff #40719)

That's not the place for doxygen comments

This revision is now accepted and ready to land.Jun 10 2023, 19:21