Add optional param returnPushdata which will cause consumeNextPush to return {push, pushdata} instead of push
Details
- Reviewers
Fabien - Group Reviewers
Restricted Project - Commits
- rABC926f1357d20e: [ecash-script] Add option to return pushdata to existing consumeNextPush…
npm test
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- return-pushdata-option
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 23979 Build 47568: Build Diff ecash-script-tests Build 47567: arc lint + arc unit
Event Timeline
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.
Change consumeNextPush to always return an object instead of a string, change version number to reflect breaking change
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 |
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 ? |
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 ... 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 |
One more comment and it should be good to go
modules/ecash-script/src/script.js | ||
---|---|---|
101 | Wow, look at how much cleaner this code is now! | |
110 | 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. |
modules/ecash-script/src/script.js | ||
---|---|---|
106 ↗ | (On Diff #40719) | That's not the place for doxygen comments |