Page MenuHomePhabricator

[ecashaddrjs] Adding existing library to monorepo as-is
ClosedPublic

Authored by bytesofman on Mar 17 2023, 23:07.

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Commits
rABC861bfe852d10: [ecashaddrjs] Adding existing library to monorepo as-is
Summary

T2848

This repo is used by exchanges working with ecash. Should be maintained and tracked in the monorepo.

Test Plan

Compare to existing master branch of codebase at https://github.com/BytesOfMan/ecashaddrjs, same except .git folder is removed

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 22680
Build 44980: Build Diff
Build 44979: arc lint + arc unit

Event Timeline

Fabien requested changes to this revision.Mar 24 2023, 14:56
Fabien added a subscriber: Fabien.

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

This revision now requires changes to proceed.Mar 24 2023, 14:56

Oh also please fix the linter issues

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

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.

bytesofman marked 7 inline comments as done.

responding to feedback

This revision is now accepted and ready to land.Mar 25 2023, 12:51