Page MenuHomePhabricator

[alias-server] add consume function in new script parsing file
ClosedPublic

Authored by bytesofman on May 30 2023, 18:02.

Details

Summary

T3060

Per feedback from D13916, adding consume method to new file script.js with unit tests.

Going forward, we may want to abstract this to a library. However there isn't a logical place to put it at the moment, and alias-server will the be first production user. Makes sense to keep it here and reserve the option to abstract to a libary later when additional use cases are known.

Test Plan

npm test

Diff Detail

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

Event Timeline

bytesofman updated this revision to Diff 40501.

More function commenting

Fabien requested changes to this revision.May 30 2023, 18:35
Fabien added a subscriber: Fabien.
Fabien added inline comments.
apps/alias-server/src/script.js
12 ↗(On Diff #40502)

This is not really the expected API for a consume function, because it does not consume anything.
You should remove the consumed bytes from the input string:

var input = 'foobar';

var consumed = consume(input, 3);
console.log(consumed); // prints 'foo'
console.log(input); // prints 'bar'

var consumed = consume(input, 3);
console.log(consumed); // prints 'bar'
console.log(input); // prints ''

var consumed = consume(input, 3); // raises an error
16 ↗(On Diff #40502)

Why ?

22 ↗(On Diff #40502)

What's the point of the one-byte hex string ? That's a super weird API.
My guess is that you're already mixing up the consume facility with the pushdata decoding.

This revision now requires changes to proceed.May 30 2023, 18:35
bytesofman marked 3 inline comments as done.

Simplifying

Tail of the build log:

/work/apps/alias-server /work/abc-ci-builds/alias-server-tests
npm WARN deprecated request-promise@4.2.6: request-promise has been deprecated because it extends the now deprecated request package, see https://github.com/request/request/issues/3142
npm WARN deprecated har-validator@5.1.5: this library is no longer supported
npm WARN deprecated uuid@3.4.0: Please upgrade  to version 7 or higher.  Older versions may use Math.random() in certain circumstances, which is known to be problematic.  See https://v8.dev/blog/math-random for details.
npm WARN deprecated request@2.88.2: request has been deprecated, see https://github.com/request/request/issues/3142

added 601 packages, and audited 602 packages in 8s

97 packages are looking for funding
  run `npm fund` for details

4 moderate severity vulnerabilities

Some issues need review, and may require choosing
a different dependency.

Run `npm audit` for details.

> alias-server@1.0.0 test
> mocha --reporter mocha-junit-reporter --reporter-options mochaFile=test_results/alias-server-junit.xml --reporter-options testsuitesTitle=Alias Server Unit Tests --reporter-options rootSuiteTitle=Alias Server

----------------------|---------|----------|---------|---------|-------------------------------
File                  | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s             
----------------------|---------|----------|---------|---------|-------------------------------
All files             |   80.79 |    84.94 |   85.71 |   80.81 |                               
 alias-server         |     100 |      100 |     100 |     100 |                               
  config.js           |     100 |      100 |     100 |     100 |                               
  secrets.sample.js   |     100 |      100 |     100 |     100 |                               
 alias-server/src     |   80.67 |    84.94 |   85.71 |   80.69 |                               
  alias.js            |   96.49 |    95.83 |     100 |   96.29 | 108-111                       
  chronik.js          |   64.17 |    83.33 |   71.42 |   65.07 | 19,44,72,113-118,151-196      
  chronikWsHandler.js |   70.37 |       20 |   66.66 |   70.37 | 23,78-94                      
  db.js               |   83.33 |       90 |     100 |   83.33 | 67-68,109-110,122-123,154-158 
  events.js           |      88 |    93.75 |     100 |   87.75 | 29-34,90-95,111,183           
  main.js             |   55.17 |     62.5 |      50 |   55.17 | 40,58-80                      
  rpc.js              |   93.75 |    83.33 |     100 |   93.75 | 36                            
  script.js           |     100 |      100 |     100 |     100 |                               
  telegram.js         |      80 |      100 |      75 |      80 | 20                            
  utils.js            |     100 |      100 |     100 |     100 |                               
----------------------|---------|----------|---------|---------|-------------------------------

##teamcity[blockOpened name='Code Coverage Summary']
##teamcity[buildStatisticValue key='CodeCoverageAbsBCovered' value='265']
##teamcity[buildStatisticValue key='CodeCoverageAbsBTotal' value='328']
##teamcity[buildStatisticValue key='CodeCoverageAbsRCovered' value='79']
##teamcity[buildStatisticValue key='CodeCoverageAbsRTotal' value='93']
##teamcity[buildStatisticValue key='CodeCoverageAbsMCovered' value='36']
##teamcity[buildStatisticValue key='CodeCoverageAbsMTotal' value='42']
##teamcity[buildStatisticValue key='CodeCoverageAbsLCovered' value='257']
##teamcity[buildStatisticValue key='CodeCoverageAbsLTotal' value='318']
##teamcity[blockClosed name='Code Coverage Summary']
Build alias-server-tests failed with exit code 1
Fabien requested changes to this revision.May 31 2023, 15:06
Fabien added inline comments.
apps/alias-server/src/script.js
17 ↗(On Diff #40505)

Why do you need to do a copy here ? Can't you work on stack directly ?

29 ↗(On Diff #40505)

What if stack length is odd ? You certainly want to avoid that case

This revision now requires changes to proceed.May 31 2023, 15:06
bytesofman marked an inline comment as done.

Responding to feedback and improving input validation

This revision is now accepted and ready to land.Jun 1 2023, 07:27