Page MenuHomePhabricator

[ecash-lib] Add `pushNumberOp` to push minimally encoded numbers
ClosedPublic

Authored by m4ktub on Jul 24 2024, 00:41.

Details

Reviewers
tobias_ruck
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Commits
rABC1c76802d8cf4: [ecash-lib] Add `pushNumberOp` to push minimally encoded numbers
Summary

Add a companion function to pushBytesOp that encodes numbers. It handles signs and little-endian conversion by replicating the CScriptNum serialization algorithm.

Depends on D16529

Test Plan

npm run test

Diff Detail

Event Timeline

Owners added a reviewer: Restricted Owners Package.Jul 24 2024, 00:41
m4ktub requested review of this revision.Jul 24 2024, 00:41
tobias_ruck added inline comments.
modules/chronik-client/package-lock.json
42 ↗(On Diff #48811)

I would keep this out of the diff, if I run npm ci it doesn't update the dependency.

modules/ecash-lib/src/op.test.ts
334 ↗(On Diff #48811)

wouldn't it be much clearer to have all the numbers in the tests in hex instead of decimal?

modules/ecash-lib/src/op.ts
132 ↗(On Diff #48811)

I know the original C++ uses these micro names, but IMO it makes it really hard to read—for example bn and 0n look related (n suffix) but that's just a naming thing.

143 ↗(On Diff #48811)

I think a few comments on what's going on here might be helpful since this is quite an obscure integer encoding

This revision now requires changes to proceed.Jul 24 2024, 00:49

Removed package-lock.json added by mistake.

m4ktub marked an inline comment as done.Jul 24 2024, 01:12
m4ktub added inline comments.
modules/ecash-lib/src/op.test.ts
334 ↗(On Diff #48811)

I had considered it and chose decimal because the makes the distinction between input and output clearer. The input is a regular number and the output its byte encoding. It's not critical and using hex will make the edge cases being tested more evident. I'll change it.

modules/ecash-lib/src/op.ts
143 ↗(On Diff #48811)

I can add comments on the obscure cases but wouldn't a note saying that it replicates src/script/script.h$363 be sufficient?

modules/ecash-lib/src/op.ts
143 ↗(On Diff #48811)

Yes, adding comments + that note would be great.

Keep in mind that this is going to be for JS/TS devs, for which even the simplest byte ops are black magic, so the code should be *clearer* than that of the C++ node

m4ktub planned changes to this revision.Jul 24 2024, 01:45
m4ktub marked 4 inline comments as done.

Added inline comments to pushNumberOp after making some variable names more legible. Represent test case numbers in hexadecimal.

tobias_ruck added inline comments.
modules/ecash-lib/src/op.ts
132 ↗(On Diff #48816)

number is already a type and it looks a bit weird in the syntax highlighting...

142 ↗(On Diff #48816)

why use number here, not value?

This revision now requires changes to proceed.Jul 24 2024, 09:07
m4ktub planned changes to this revision.Jul 24 2024, 16:42
m4ktub marked 2 inline comments as done.
m4ktub added inline comments.
modules/ecash-lib/src/op.ts
132 ↗(On Diff #48816)

I agree that it's a bit weird. Naming is not easy... I'll prioritize the argument name and use value for it, because it avoids abbreviations and it's visible in autocompletes. The internal bigint value will get some other name.

142 ↗(On Diff #48816)

Good catch. No particular reason except comparison operators working without needing a cast.

m4ktub marked 2 inline comments as done.

Rename function argument to value and move internal bigint down with a different name.

tobias_ruck added inline comments.
modules/ecash-lib/src/op.test.ts
302 ↗(On Diff #48825)

maybe add a few more tests here (especially all the "boundaries" from n bytes to n + 1 bytes) but otherwise LGTM

This revision is now accepted and ready to land.Jul 24 2024, 19:20

Rebasing to include latest merges.