Page MenuHomePhabricator

[ecash-lib] Add `Op`, opcode consts, readOp, writeOp
ClosedPublic

Authored by tobias_ruck on Apr 10 2024, 20:45.

Details

Reviewers
bytesofman
Group Reviewers
Restricted Project
Commits
rABCaac89ce3e7df: [ecash-lib] Add `Op`, opcode consts, readOp, writeOp
Summary

Basic functionality to read and write opcodes.

Opcode is a number and not an enum for two reasons:

  1. Having to type Opcode.OP_CHECKSIG every time instead of OP_CHECKSIG is much simpler
  2. The opcode list isn't exhaustive, e.g. 0xd0 would be a valid opcode, it would just fail immediately if in an executed branch.

Also, for pushops, the used opcode is explicit and not automatically chosen, this is because there's different usages require different rules:

  1. In executed Bitcoin script, all pushops must be encoded minimally, e.g. the empty string must be pushed using OP_0, not using OP_PUSHDATA1
  2. In SLP, single push opcodes like OP_0 cannot be used, so to push the empty string, OP_PUSHDATAn must be used in this case.

In future diffs, we can add functions to get us minimal Ops for either of these cases.

Test Plan

npm run test && npm run build

Diff Detail

Repository
rABC Bitcoin ABC
Branch
ecash-lib-op
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 28456
Build 56453: Build Diff
Build 56452: arc lint + arc unit

Event Timeline

nits + rerun CI

modules/ecash-lib/src/op.test.ts
73

is there any standard on p2pkh vs P2PKH?

ecashaddrjs does capitalized, chronik requires lowercase

unless there is some important reason use one or the other, i vote we just start using p2pkh (lowercase). This is how we currently handle hex strings (just use lowercase tho technically uppercase is also ok).

86
172
183
modules/ecash-lib/src/op.ts
10–11
29

we don't check that the values at opcode and data are of the expected type? how will this function be used in context?

This revision now requires changes to proceed.Apr 11 2024, 03:51
modules/ecash-lib/src/op.test.ts
73

I usually use P2PKH (as it stands out more) in comments and p2pkh in constants/paths, as it’s more interoperable.

I don’t really care about the comment here, but I think using P2PKH is a tiny bit clearer here as it stands out more.

I do think having a standard on anything that’s not a comment is important and it should be the lowercase version

modules/ecash-lib/src/op.test.ts
73

👌

modules/ecash-lib/src/op.ts
29

tbh it's just to distinguish the two variants of Op, Opcode and PushOp in TypeScript. Using if (isPushOp(x)) { /* x is PushOp here */ } makes it TypeScript knows which of the two variants we're dealing with.

It's not really meant to be perfect catchall, but I can test the entire thing since someone might end up using it and feel bamboozled.

fix comments, make isPushOp also check for types

This revision is now accepted and ready to land.Apr 11 2024, 22:47