Page MenuHomePhabricator

Adding the explorer to the monorepo
ClosedPublic

Authored by johnkuney on Dec 14 2022, 17:27.

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Commits
rABCfa8e107f61e6: Adding the explorer to the monorepo
Summary

As requested, adding the explorer.e.cash repo to the mono repo

Test Plan

Open the explorer folder in the web directory. Try running the app (see the readme for instructions)

Diff Detail

Repository
rABC Bitcoin ABC
Branch
add-explorer
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 21600
Build 42841: Build Diff
Build 42840: arc lint + arc unit

Event Timeline

Fabien requested changes to this revision.Dec 14 2022, 17:41
Fabien added a subscriber: Fabien.

Please fix the various lint issues that are reported here

This revision now requires changes to proceed.Dec 14 2022, 17:41
web/explorer/explorer-server/code/webix/webix.js
5 ↗(On Diff #37298)

That is incompatible with our MIT license

Looks like the "webix" and "semantic-ui" files probably don't belong in our repository.
Seems like they are libraries, can they be linked externally?

Also, the Webix license is GPL so not compatible with our codebase (MIT). It will probably need to be switched out for something else.

Okay I'll see about getting rid of those

Fabien requested changes to this revision.Dec 14 2022, 19:53

Clearing the queue since more work has to be done

This revision now requires changes to proceed.Dec 14 2022, 19:53

removed semantic-ui and webix and updated readme

@Fabien any ideas what this lint error is about?

Screen Shot 2022-12-14 at 3.59.24 PM.png (784×1 px, 170 KB)

Yes, the error title is: Line limit exceeded, that's probably a good hint :)

web/explorer/explorer-server/src/templating/filters.rs
195 ↗(On Diff #37316)
Fabien requested changes to this revision.Dec 15 2022, 08:47
Fabien added inline comments.
web/explorer/README.md
11 ↗(On Diff #37316)

4 levels !?

15 ↗(On Diff #37316)

Looks like you want to supply an environment variable or similar that points to the bitcoinsuite directory

64 ↗(On Diff #37316)

Which libraries ?

70 ↗(On Diff #37316)

IMO this doesn't belong in a README. There are a ton of ways to run the explorer systemd is only one such example. Also in the service unit is better provided in a sample file rather than in a markdown code block.

112 ↗(On Diff #37316)

That's kinda expected isn't it ?

web/explorer/explorer-exe/Cargo.toml
8 ↗(On Diff #37316)

see above comment

web/explorer/explorer-server/Cargo.toml
10 ↗(On Diff #37316)

dito

web/explorer/explorer-server/assets/known_bugs.md
1 ↗(On Diff #37316)

I don't think this file makes sense, the bugs should be in a bug tracker (and there are much more known bugs than this one, see github issues)

web/explorer/explorer-server/assets/ludwig.png
1 ↗(On Diff #37316)

What is this used for ?

web/explorer/explorer-server/code/address.js
68 ↗(On Diff #37316)

Don't put commented code in the repo

This revision now requires changes to proceed.Dec 15 2022, 08:47

remove some unneeded files, commented out code, and readme instructions

Fabien requested changes to this revision.Dec 15 2022, 18:20
Fabien added inline comments.
web/explorer/README.md
11

This still needs to be fixed

web/explorer/explorer-server/assets/network-background2.png
1

Why is there 2 background files ?

web/explorer/explorer-server/code/blocks.js
158

?

web/explorer/explorer-server/code/common.js
39

Remove

58

That's not in par with the style for other if statements. Maybe it deserves a prettier config adjustment ?

145

Please go over the whole code and clean all the parts that are commented out.
Extra bonus point if prettier (or another linter) can take care of this automatically.

web/explorer/explorer-server/code/moment.min.js
1

This is an external library and afaict it's missing the associated license file/copyright

web/explorer/explorer-server/code/styles/index.css
192–193

Remove

852

dito, and in various other places

This revision now requires changes to proceed.Dec 15 2022, 18:20

Remove commented code and external library

Can you explain more specifically what you want to do with the bitcoinsuite-chronik-client?

web/explorer/README.md
15 ↗(On Diff #37316)

isnt that what this is?

web/explorer/explorer-server/assets/network-background2.png
1

One is a bit different and used in a different place

web/explorer/README.md
15 ↗(On Diff #37316)

The path is hardcoded to be 4 levels above. if you can supply some environment variable with the path to the bitcoinsuite/ directory, (e.g. called BITCOINSUITE_DIR), you can do something like:

bitcoinsuite-chronik-client = { path = "$BITCOINSUITE_DIR/bitcoinsuite-chronik-client" }

(of course the syntax is wrong) in the Cargo.toml and you remove the nesting requirements for the parent directories, only the children needs to match the expectations which is fine.

Okay for sure makes sense. If you find any references for the proper syntax to do something like that let me know...I'm not sure how to write it

Im guessing would be ideal to define that variable in the config.toml right?

Im not sure its doable @Fabien. I asked Tobias but he didnt have a solution, and I cant find anything about using environmental variable like that...Tobias did say they are trying to make the bitcoinsuite a cargo crate which will be even cleaner...

Fabien requested changes to this revision.Dec 16 2022, 10:14
Fabien added inline comments.
web/explorer/explorer-server/Cargo.toml
8 ↗(On Diff #37328)

I'll look into a better solution.
In order to move forward, can't we at least avoid the path= mechanism and simply use the git= scheme instead ? See suggestion.

web/explorer/explorer-server/templates/components/output.html
46 ↗(On Diff #37328)

You missed some

This revision now requires changes to proceed.Dec 16 2022, 10:14

Remove more commented out lines

Fabien requested changes to this revision.Dec 21 2022, 08:36

The README remains to be updated, but otherwise it's good to go

web/explorer/README.md
18 ↗(On Diff #37401)

That paragraph is no longer true

This revision now requires changes to proceed.Dec 21 2022, 08:36

Update Readme to remove the bitcoinsuite path info

This revision is now accepted and ready to land.Dec 21 2022, 16:11
This revision was automatically updated to reflect the committed changes.