[chronik] bump karyon_jsonrpc
AcceptedPublic
Actions

Authored by PiRK on Wed, Jan 8, 18:52.

Details

Reviewers

Fabien

Group Reviewers

Restricted Project

Summary

The latest commit on the master branch of the karyon repo changes RPC errors to accept non-static strings. This enables more helpful error messages in RPC responses.

In this diff we bump the version and fix our code to account for this breaking change in API.

Test Plan

ninja check-functional

Diff Detail

Repository

rABC Bitcoin ABC

Branch

custom_error

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 31988
Build 63468: Build Diff	chronik-client-integration-tests · build-chronik-plugins · build-chronik · build-explorer · ecash-lib-integration-tests · ecash-agora-integration-tests
Build 63467: arc lint + arc unit

Event Timeline

PiRK created this revision.Wed, Jan 8, 18:52

Herald added a reviewer: Restricted Project. · View Herald TranscriptWed, Jan 8, 18:52

PiRK requested review of this revision.Wed, Jan 8, 18:52

PiRK edited the summary of this revision. (Show Details)Wed, Jan 8, 18:54

nit: use format! rather than concat! and save a to_string() conversion

same

Harbormaster completed remote builds in B31981: Diff 52049.Wed, Jan 8, 19:17

Harbormaster completed remote builds in B31982: Diff 52050.

Harbormaster completed remote builds in B31983: Diff 52051.Wed, Jan 8, 19:26

Couple nits but otherwise looks good

chronik/chronik-http/src/electrum.rs
7 ↗	(On Diff #52051)	I'm surprised this is needed
364 ↗	(On Diff #52051)	can't you pass the string directly here ? and avoid the copy

This revision now requires changes to proceed.Wed, Jan 8, 19:51

PiRK added inline comments.Thu, Jan 9, 07:12

chronik/chronik-http/src/electrum.rs
7 ↗	(On Diff #52051)	I need to tell my IDE to stop adding imports automatically. I saw it in the diff and assumed that it was actually needed.
364 ↗	(On Diff #52051)	In that case there will be two `.to_string()` conversions, here and below on line 375. In terms of performance it will be equivalent, but the string will be duplicated in the source code which imo is a bit more error prone.

remove unneeded import

Harbormaster completed remote builds in B31988: Diff 52059.Thu, Jan 9, 07:12

teamcity added a comment.Thu, Jan 9, 07:12

This comment was removed by Fabien.

PiRK added inline comments.Thu, Jan 9, 07:25

chronik/chronik-http/src/electrum.rs
364 ↗	(On Diff #52051)	I think the clone would technically be unnecessary because if this error is returned then the following code is unreachable. Maybe rewriting this with an explicit `if` branch would solve this, if the compiler is smart enough to recognize that the various code branches are mutually exclusive ?

PiRK added a child revision: D17485: [chronik] implement blockchain.block.header electrum method.Thu, Jan 9, 07:58

PiRK mentioned this in D17370: [chronik] add blockchain.transaction.broadcast electrum method.Thu, Jan 9, 08:04

Fabien accepted this revision.Thu, Jan 9, 10:03

Fabien added inline comments.

chronik/chronik-http/src/electrum.rs
364 ↗	(On Diff #52051)	if you're getting an error here the clone is not that bad, let's not try to over optimize
364 ↗	(On Diff #52051)	Oh I missed it was reused, fine then