Page MenuHomePhabricator

[Modules] Added `bitcoinsuite-chronik-client` to monorepo
Needs ReviewPublic

Authored by hazzarust on Wed, Dec 11, 15:59.

Details

Reviewers
bytesofman
tobias_ruck
Fabien
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Summary

Integration into the CI will happen in the following diffs. This is to confirm structure of bitcoinsuite-chronik-client within modules. This is a client module written in Rust for interacting with the Chronik API.

Added "modules/bitcoinsuite-chronik-client" to the main Cargo.toml file bitcoin-abc/Cargo.toml so that Rust analyser works in bitcoinsuite-chronik-client

Testing comments to cover what each test is doing will be put forward in a seperate diff as not part of original source material.

Deleted the proto file as we can use the proto build from chronik/chronik-proto

Test Plan

ninja check-functional
run cargo test in bitcoin-abc/modules/bitcoinsuite-chronik-client/tests/
contrib/teamcity/build-configurations.py build-bitcoinsuite-chronik-client

Diff Detail

Repository
rABC Bitcoin ABC
Branch
chronik_rust_client
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 31617
Build 62731: Build Diff
Build 62730: arc lint + arc unit

Event Timeline

Owners added a reviewer: Restricted Owners Package.Wed, Dec 11, 15:59
bytesofman added a subscriber: bytesofman.

nice.

is this a greenfield lib or is it initializing a repo in the monorepo that exists somewhere else?

modules/bitcoinsuite-chronik-client/Cargo.toml
32

newline

modules/bitcoinsuite-chronik-client/proto/chronik.proto
515

newline

modules/bitcoinsuite-chronik-client/tests/test_chronik_client.rs
1

This file is testing the lib?

Does the test plan of this diff run these tests?

It's difficult to review these tests. Seems like there are a lot of tests, but not a lot of language to show what the tests are demonstrating.

148

mb something like this? I'm not sure how tests are usually organized in rust. but should be some way for a reviewer to see what is being tested without translating the code for every test.

This revision now requires changes to proceed.Wed, Dec 11, 19:17
tobias_ruck added a subscriber: tobias_ruck.
tobias_ruck added inline comments.
modules/bitcoinsuite-chronik-client/build.rs
4

Let’s point this to /chronik/chronik-proto/proto/chronik.proto right away

modules/bitcoinsuite-chronik-client/src/lib.rs
26

Let’s remove these two P2tr already

nice.

is this a greenfield lib or is it initializing a repo in the monorepo that exists somewhere else?

This is Tobias' client, the one we use for the explorer. @hazzarust is putting it into the monorepo then will add the failover proxy to it.

nice.

is this a greenfield lib or is it initializing a repo in the monorepo that exists somewhere else?

This is Tobias' client, the one we use for the explorer. @hazzarust is putting it into the monorepo then will add the failover proxy to it.

ok nice

in that case, not really a big deal about adding stuff like test comments, having this as a starting point is helpful

nice.

is this a greenfield lib or is it initializing a repo in the monorepo that exists somewhere else?

This is Tobias' client, the one we use for the explorer. @hazzarust is putting it into the monorepo then will add the failover proxy to it.

ok nice

in that case, not really a big deal about adding stuff like test comments, having this as a starting point is helpful

Sorry, I should of been way more explicit about this in the summary of this diff. Comments for tests, and test integration for CI is coming soon!

Updated everything in this diff - please see summary for more indepth

Deleted 'proto' file from bitconsuite-chronik-client, as up to date on exists within bitcoin-abc/chronik/chronik-proto/proto/chronik.proto that we can use.

hazzarust edited the test plan for this revision. (Show Details)
tobias_ruck added inline comments.
chronik/bitcoinsuite-core/src/bytes.rs
7 ↗(On Diff #51572)

Don’t do this, instead just import from the dependency directly where needed

modules/bitcoinsuite-chronik-client/Cargo.toml
1 ↗(On Diff #51572)

Add a copyright notice

modules/bitcoinsuite-chronik-client/build.rs
1 ↗(On Diff #51572)

I think we should just use chronik-proto instead of compiling again

modules/bitcoinsuite-chronik-client/src/lib.rs
2 ↗(On Diff #51572)

Instead of compiling the proto again, maybe we can already just import chronik-proto from /chronik

modules/bitcoinsuite-chronik-client/tests/test_chronik_client.rs
17 ↗(On Diff #51572)

Actually fill in the comment or remove it

This revision now requires changes to proceed.Fri, Dec 13, 18:19
hazzarust edited the summary of this revision. (Show Details)

Import chronik-proto from /chronik

Deleted newline in hash.rs

tobias_ruck added inline comments.
modules/bitcoinsuite-chronik-client/Cargo.toml
27 ↗(On Diff #51581)

move it up to the other local dependencies:) frens need frens

This revision now requires changes to proceed.Sat, Dec 14, 13:29
Cargo.toml
25 ↗(On Diff #51581)

sort this to the other modules/

Moved modules/bitcoinsuite-chronik-client next to other modules in workspace. Moved chronik-proto = { path = "../../chronik/chronik-proto/"} to other local dependencies

Fabien requested changes to this revision.Mon, Dec 16, 09:52
Fabien added inline comments.
modules/bitcoinsuite-chronik-client/tests/test_chronik_client.rs
12 ↗(On Diff #51591)

This is problematic. We don't test against production servers because there is no way to make sure the version is synced, and this subject to failures if the server goes down.
The solution is to build and run a server on the regtest chain (cpu mined chain for regression testing) and test against that using the local network. This is what is done for the ts client for example, leveraging the node test framework. This should be fixed in another diff.

But for now using Tobias' production server is not a good option. You can use the public ABC node instead https://chronik.e.cash until the root issue is fixed.

This revision now requires changes to proceed.Mon, Dec 16, 09:52
tobias_ruck added inline comments.
modules/bitcoinsuite-chronik-client/src/lib.rs
1 ↗(On Diff #51591)

add the (standard) copyright (I'm the author anyway so it's fine)

8–9 ↗(On Diff #51591)

see suggestion below

64 ↗(On Diff #51591)

I used to do that but we should really move this into the use statements above (see suggestion in the top)

modules/bitcoinsuite-chronik-client/tests/test_chronik_client.rs
1 ↗(On Diff #51591)

same here

17 ↗(On Diff #51591)

Can you try just using #[tokio::test] for as many as possible here? That should simplify the code some more

40–47 ↗(On Diff #51591)

let's remove this

Added CI integration (build-configurations.yml), added copyright licenses, changed server and reduced Tokio syntax.

@bot build-bitcoinsuite-chronik-client