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
Details
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
npm run test
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- ecash-lib-pushnumberop
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 29710 Build 58955: Build Diff ecash-lib-integration-tests · ecash-lib-tests · ecash-agora-integration-tests Build 58954: arc lint + arc unit
Event Timeline
| 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 |
| 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 |
Comment Actions
Added inline comments to pushNumberOp after making some variable names more legible. Represent test case numbers in hexadecimal.
| 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. |
Comment Actions
Rename function argument to value and move internal bigint down with a different name.
| 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 |