T2848
This repo is used by exchanges working with ecash. Should be maintained and tracked in the monorepo.
Details
Details
- Reviewers
Fabien - Group Reviewers
Restricted Project - Commits
- rABC861bfe852d10: [ecashaddrjs] Adding existing library to monorepo as-is
Compare to existing master branch of codebase at https://github.com/BytesOfMan/ecashaddrjs, same except .git folder is removed
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 22670 Build 44960: Build Diff Build 44959: arc lint + arc unit
Event Timeline
Comment Actions
The main issues I see here are:
- The use/assumption of external tools such as bower and yarn that we don't use in other parts of the project, and we don't install on CI
- The dist/ folder should not be part of a source tree. These are the byproduct of the packaging process and should even be added to the .gitignore
Other minor issue:
- Various references to the upstream github repo remain, but some are updated. This should be consistent across the files
| web/ecashaddrjs/.gitignore | ||
|---|---|---|
| 5 ↗ | (On Diff #38688) | I don't think we use yarn in the monorepo ? |
| 33 ↗ | (On Diff #38688) | build/* so other configs are ignored as well |
| 55 ↗ | (On Diff #38688) | dito |
| web/ecashaddrjs/.mocharc.json | ||
| 3 ↗ | (On Diff #38688) | We don't use mocha anywhere else afaik, but it's fine for a first commit |
| web/ecashaddrjs/.travis.yml | ||
| 5 ↗ | (On Diff #38688) | we don't use travis |
| web/ecashaddrjs/LICENSE | ||
| 3 ↗ | (On Diff #38688) | Should add ours as well |
| web/ecashaddrjs/README.md | ||
| 9 ↗ | (On Diff #38688) | not worth keeping |
| 21 ↗ | (On Diff #38688) | The name should be updated, and the versioned files don't belong to the sources. This can be handled in follow ups. |
| 69 ↗ | (On Diff #38688) | note that an address without a prefix is actually a valid address |
| web/ecashaddrjs/bower.json | ||
| 18 ↗ | (On Diff #38688) | we don't use bower either |
| web/ecashaddrjs/dist/cashaddrjs-1.0.7.min.js | ||
| 1 ↗ | (On Diff #38688) | that' not minified ? |
| web/ecashaddrjs/dist/cashaddrjs-1.0.7.min.js.LICENSE.txt | ||
| 4 ↗ | (On Diff #38688) | dito |
| web/ecashaddrjs/package.json | ||
| 14 ↗ | (On Diff #38688) | dito |
| 30 ↗ | (On Diff #38688) | add eCash |
| web/ecashaddrjs/src/cashaddr.js | ||
| 69 ↗ | (On Diff #38688) | Not for this diff: you have a list of valid prefixes, you can easily use the checksum to find the right one if not supplied and only request it for other cash address formats |
| web/ecashaddrjs/test/base32.js | ||
| 8 ↗ | (On Diff #38688) | useful |
| web/ecashaddrjs/test/convertBits.js | ||
| 8 ↗ | (On Diff #38688) | dito |
Comment Actions
The use/assumption of external tools such as bower and yarn that we don't use in other parts of the project, and we don't install on CI
Yes, these should be removed (and are later in the stack). However I think it is important to start in the same place as the current prod published version.
The dist/ folder should not be part of a source tree. These are the byproduct of the packaging process and should even be added to the .gitignore
Yes, it shouldn't be there. However, this was the standard used in the preceding repo. So, it should be a different task to remove it here. Catalogued in
Various references to the upstream github repo remain, but some are updated. This should be consistent across the files
Also true. But also separate task. This diff is just starting from exactly what the published version is right now.
These issues are tracked in this task, to be completed before version bump and publishing of updated ecashaddrjs: Various references to the upstream github repo remain, but some are updated. This should be consistent across the files
Comment Actions
Your inline comments are good and should be addressed. But I think it's easier to see them in later diffs that are changing this already-dumped codebase, rather than dumping+modifying in one diff.
I have them listed as a to-do item in T3039 before the next version of ecashaddrjs is published.