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
Branch
add-script-and-consume
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 23842
Build 47294: Build Diffalias-server-tests
Build 47293: arc lint + arc unit

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