Details

Reviewers

bytesofman
Fabien

Group Reviewers

Restricted Project

Commits

rABC1ceb096d1a1c: [Apps] [Examples] Initial mocha framework and getDetailsFromTxid example

Summary

T3203

This diff creates an Example folder under Apps on the monorepo and which will be populated with example code as a reference guide for new devs looking into building on eCash.

This is an initial diff containing the readme and the getDetailsFromTxid example along with the mocha test suite initialized.

Test Plan

nvm install 16
npm i
npm run getDetailsFromTxid <chronik url><txid> and ensure details for the txid is displayed and matches the block explorer
npm test and ensure all tests pass

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Herald added a reviewer: Restricted Project. · View Herald TranscriptJun 22 2023, 03:53

emack requested review of this revision.Jun 22 2023, 03:53

Harbormaster completed remote builds in B24128: Diff 40921.Jun 22 2023, 03:53

OK with the idea, however the examples should be tested on CI or you can be sure they will be broken in no time and will then serve no purpose.

apps/examples/README.md
19 ↗	(On Diff #40921)	Is it possible to use args rather than environment variables ? Like `node getDetailsFromTxid.js <txid>`

Switched from env vars to args in usage example. Will add this to CI shortly.

Harbormaster completed remote builds in B24132: Diff 40926.Jun 22 2023, 08:41

emack edited the test plan for this revision. (Show Details)Jun 22 2023, 08:42

emack planned changes to this revision.Jun 22 2023, 14:00

Added mocha tests and chronik mocks

Harbormaster completed remote builds in B24134: Diff 40929.Jun 22 2023, 14:36

emack planned changes to this revision.Jun 22 2023, 14:48

Added mock for the chronik.tx() call inside getTxDetails
Added unit test to CI by following D14012 # how do I verify this other than waiting for the nightly builds?

Harbormaster completed remote builds in B24143: Diff 40941.Jun 23 2023, 11:10

emack retitled this revision from [Apps] [Examples] Initial folder and getDetailsFromTxid example to [Apps] [Examples] Initial mocha framework and getDetailsFromTxid example.Jun 23 2023, 11:11

emack edited the summary of this revision. (Show Details)

typo

Harbormaster completed remote builds in B24144: Diff 40942.Jun 23 2023, 11:19

emack edited the test plan for this revision. (Show Details)Jun 23 2023, 11:21

We probably want CI here to run the actual scripts and not just a typical mocha unit test implementation (opposite of what I suggested earlier, sorry). So,

Refactor what you have now so that getDetailsFromTxid.js is a callable npm script, i.e. npm run getDetailasFromTxid <txid> will work
We'll examine how to get CI to run the actual script. test cases will be different txids or "no argv runs default", etc

apps/examples/getDetailsFromTxid.js
2 ↗	(On Diff #40942)	We'll need a config.js to keep info like this
27 ↗	(On Diff #40942)	We don't want to export a function call. Also, each script should have a certain level of "how to use this" documentation.

This revision now requires changes to proceed.Jun 23 2023, 15:38

Updated package.json to turn this into a callable npm script npm run getDetailsFromTxid <txid>
Refactored example into scripts containing the live usage, test containing the mockChronik tests and src containing the function itself
Expanded tests to cover eToken IDs and no arg supplied scenarios
Added additional usage documentation to script
Created separate config.js

Harbormaster completed remote builds in B24158: Diff 40964.Jun 24 2023, 06:30

emack edited the test plan for this revision. (Show Details)Jun 24 2023, 06:30

emack edited the test plan for this revision. (Show Details)Jun 24 2023, 06:34

Fabien requested changes to this revision.Jun 24 2023, 14:54

Fabien added inline comments.

apps/examples/mocks/chronikMock.js
6 ↗	(On Diff #40964)	the comment is misplaced, useless and has a bad layout. Combo !
apps/examples/scripts/getTxDetailsFromTxid.js
13 ↗	(On Diff #40964)	Move the comment above the call. This line should not change if the default is changed, but the comment does.
apps/examples/src/tx.js
22 ↗	(On Diff #40964)	That makes zero sense. So now this function does random thing depending on what you're supplying. This is supposed to be an example: keep it simple and don't bloat with test related stuff that is totally out of concern for a dev looking at an example. Why should I care about the default chronik instance ? If I need a chronik instance to run the function, then show me how to do it instead of hiding half of the code. Why is there a default txid ? Is this some special value ? If a value is needed for testing, just pass it in the test. Why is there 2 outputs ??? This is an example function returning tx details, it should not print anything and throw an error. You can print whatever you want at callsite, whether this is the wrapper cli function or the test. This is an example: ask yourself if this is what such a similar function should look like in your real application.

This revision now requires changes to proceed.Jun 24 2023, 14:54

Why is there 2 outputs?

Good point, I've removed it from the example function and it will only output the results at callsite via the script

Why is there a default txid?

In hindsight yes, shouldn't need to hand-hold the dev, if they haven't figured out it needs txid as an arg then they need to read the readme. Have simplified by removing default txid logic throughout this diff.

Why should I care about the default chronik instance ? If I need a chronik instance to run the function, then show me how to do

The reason there's a default chronik instance is because when the dev is executing this example via npm run getDetailsFromTxid <txid> they should be hitting the prod instance by default to pull the onchain data for their txid, whereas when this example is executed by the test suite it should only be hitting the mock Chronik instance and not make any api calls. This was originally a simple example script using only the prod instance but the need for unit tests and adding to CI necessitated this additional logic.
In terms of showing the dev how to declare the chronik instance, the readme points them to tx.js which is where the chronik instance is declared.

Updated comments per feedback

Harbormaster completed remote builds in B24162: Diff 40972.Jun 25 2023, 01:10

emack edited the test plan for this revision. (Show Details)Jun 25 2023, 01:10

The reason there's a default chronik instance is because when the dev is executing this example via npm run getDetailsFromTxid <txid> they should be hitting the prod instance by default to pull the onchain data for their txid, whereas when this example is executed by the test suite it should > only be hitting the mock Chronik instance and not make any api calls. This was originally a simple example script using only the prod instance but the need for unit tests and adding to CI necessitated this additional logic.
In terms of showing the dev how to declare the chronik instance, the readme points them to tx.js which is where the chronik instance is declared.

OK so now let's look closely at your code:

displayTxDetails() is in the getTxDetailsFromTxid.js file.
but getTxDetails() is in the tx.js file, despite the above file name.

This is already be a hint that something is not quite good.

Now the functions themselves: one is just calling the other, which does a simple call to chronik:

do you think it's worth having 2 files ? AFAIK there is no rule preventing you from having 2 functions in a single file.
if I'm looking for an example, now I need 3 files, the 2 mentioned above + the config file because I have no idea what a chronik URL looks like.
and at the end, it's just a simple URL! So what I want to know is:
- what is an OK value to call my test script and eventually trace the code flow ?
- how to spin up a chronik instance so I can deploy my own infra for my app ?

The conclusion is that the chronik URL should be passed on the command line, the comment/readme should point to some sane default value so I don't have to go down the rabbit hole if I just need to run the script, as well as a doc about running a chronik instance so I can test the same script against my own infra to check it's working.

None of this is anything technical. It's just a matter of looking at the code and remember what you're trying to achieve.

apps/examples/src/tx.js
6 ↗	(On Diff #40972)	How does that work ? The chronik config is now in config/chronik.js

This revision now requires changes to proceed.Jun 25 2023, 14:06

further simplified example by consolidating all example logic in scripts/getDetailsFromTxid
chronik url is now passed from CLI e.g. npm run getDetailsFromTxid <chronik url> <txid>
will speak to @bytesofman re: a guide on spinning up a chronik instance separate to this diff

Harbormaster completed remote builds in B24169: Diff 40979.Jun 25 2023, 15:48

emack edited the test plan for this revision. (Show Details)Jun 25 2023, 15:48

updated comments

Harbormaster completed remote builds in B24170: Diff 40980.Jun 25 2023, 15:52

@bot app-dev-examples

apps/examples/scripts/getDetailsFromTxid.js
8 ↗	(On Diff #40980)	You'd better write some usage line as well here, it's just easier to read as a quick manual.
28 ↗	(On Diff #40980)	This is OK, but for the sake of it being an example and not production code I would create a txid variable first (like there is chronik variable) then call the getDetailsFromTxid(chronik, txid). You want an example to be as easy to read as possible even if that means that it's not as optimized as it could be. Not a blocker though. Are the chronik backend errors printed out ? What appends if getDetailsFromTxid raises an exception ?

teamcity awarded a token.Jun 25 2023, 18:19

Also: for now it's fine to just redirect to the Chronik NNG github README for the instructions. We can update that link when we have better doc, but for now we have to do with what we have.

added usage comments to getDetailsFromTxid
added txid variable for readability
updated README to point to the chronik NNG github README for the optional step of setting up own chronik instance

Harbormaster completed remote builds in B24171: Diff 40981.Jun 26 2023, 02:40

emack added inline comments.Jun 26 2023, 02:42

apps/examples/scripts/getDetailsFromTxid.js
28 ↗	(On Diff #40980)	yes chronik backend errors are printed out, have tested by supplying a non-existent txid and it printed chronik's `getaddrinfo ENOTFOUND` exception.

Fabien added a subscriber: tobias_ruck.Jun 26 2023, 06:21

Fabien added inline comments.

apps/examples/README.md
17 ↗	(On Diff #40981)	Did you ask @tobias_ruck if he's OK with having his handle on public websites like Github ? If not just remove the last part.
apps/examples/scripts/getDetailsFromTxid.js
30 ↗	(On Diff #40981)	nit: you don't need to slice twice

remove duplicated splice logic
Removed tg handle in readme

Harbormaster completed remote builds in B24176: Diff 40988.Jun 26 2023, 07:26

Fabien accepted this revision.Jun 26 2023, 15:16

apps/examples/test/getDetailsFromTxidTests.js

While I have been using this naming convention in the /apps/ dir, I think a better one is

getDetailsFromTxid.test.js

This is regognized and labeled as a test file by VS code, and imo is more directly recognizable as being "the test fiole for getDetailsFromTxis" while still avoiding the ambiguity issue of it having the same name.

I'm planning to migrate all the test file names in ecash-herald and alias-server to this convention. Might as well start this repo off on the right foot by initializing it here.

bytesofman requested changes to this revision.Jun 26 2023, 17:22

bytesofman added inline comments.

apps/examples/scripts/getDetailsFromTxid.js
9–15 ↗	(On Diff #40988)
apps/examples/test/getDetailsFromTxidTests.js
12 ↗	(On Diff #40988)	include a test showing it throws an error on a chronik error you can set an error as the chronik response in mockedChronik by calling passing an error object with `setMock`

This revision now requires changes to proceed.Jun 26 2023, 17:22

updated test file to align with naming convention
added new test to ensure chronik errors are being thrown

Harbormaster completed remote builds in B24193: Diff 41015.Jun 27 2023, 06:13

bytesofman accepted this revision.Jun 27 2023, 16:29

This revision is now accepted and ready to land.Jun 27 2023, 16:29

Closed by commit rABC1ceb096d1a1c: [Apps] [Examples] Initial mocha framework and getDetailsFromTxid example (authored by emack). · Explain WhyJun 28 2023, 11:48

This revision was automatically updated to reflect the committed changes.

emack added a commit: rABC1ceb096d1a1c: [Apps] [Examples] Initial mocha framework and getDetailsFromTxid example.

emack mentioned this in D14146: [Apps][Examples] addressToHash160.Jun 29 2023, 12:13

[Apps] [Examples] Initial mocha framework and getDetailsFromTxid example
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 41067

apps/examples/.gitignore

apps/examples/README.md

apps/examples/mocks/chronikMock.js

apps/examples/mocks/chronikResponses.js

apps/examples/package-lock.json

apps/examples/package.json

apps/examples/scripts/getDetailsFromTxid.js

apps/examples/test/getDetailsFromTxid.test.js

contrib/teamcity/build-configurations.yml

[Apps] [Examples] Initial mocha framework and getDetailsFromTxid exampleClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 41067

apps/examples/.gitignore

apps/examples/README.md

apps/examples/mocks/chronikMock.js

apps/examples/mocks/chronikResponses.js

apps/examples/package-lock.json

apps/examples/package.json

apps/examples/scripts/getDetailsFromTxid.js

apps/examples/test/getDetailsFromTxid.test.js

contrib/teamcity/build-configurations.yml

[Apps] [Examples] Initial mocha framework and getDetailsFromTxid example
ClosedPublic
Actions

Revision Contents
Changeset List