Add JsonSchema Support for Several Types #501

JeremyRubin · 2020-10-12T04:34:56Z

This patch adds JsonSchema support for types which don't require JsonSchema patched bitcoin_hashes or secp256k1. Those types can be added as follow up work once those jsonschema patches are merged & released, but a broad enough set of types can be implemented without any version bump that it's reasonable to do in two parts.

A new type, CoinAmount, is introduced for contexts where one might want to specify either a floating point Bitcoin or integral satoshi amount of coin. This is useful in contexts where you might be specifying a type for user input and are able to take either a Btc or Sats amount, but don't know ahead of time what the user might send; it generates the following schema.

  {
    "CoinAmount": {
      "description": "Type for dealing with unknown amounts",
      "anyOf": [
        {
          "description": "Sats type",
          "type": "object",
          "required": [
            "Sats"
          ],
          "properties": {
            "Sats": {
              "type": "integer",
              "format": "uint64",
              "minimum": 0.0
            }
          }
        },
        {
          "description": "Btc type",
          "type": "object",
          "required": [
            "Btc"
          ],
          "properties": {
            "Btc": {
              "type": "number",
              "format": "double"
            }
          }
        }
      ]
    }
  }

apoelstra · 2020-10-14T13:48:10Z

Cargo.toml

@@ -29,8 +29,8 @@ bitcoinconsensus = { version = "0.19.0-1", optional = true }
 serde = { version = "1", optional = true }

 [dev-dependencies]
-serde_derive = "<1.0.99"


I really don't understand these. Under what circumstances would cargo use a different major version than the one that is specified? Does <1.0.99 really work this way?

https://jubianchi.github.io/semver-check/#/%3C1.0.99/0.1.2

This constraint does not provide a lower bound which means you will probably get unexpected breaking changes.

Cargo.toml

jeandudey · 2020-10-16T22:02:01Z

src/util/amount.rs

+/// Derived externally using serde_derive
+#[cfg(feature = "serde")]
+#[doc(hidden)]
+#[allow(non_upper_case_globals, unused_attributes, unused_qualifications)]
+const _: () = {
+    #[allow(rust_2018_idioms, clippy::useless_attribute)]
+    extern crate serde as _serde;
+    impl _serde::Serialize for CoinAmount {
+        fn serialize<__S>(&self, __serializer: __S) -> _serde::export::Result<__S::Ok, __S::Error>


If this is auto generated by serde_derive I'd recommend cleaning it manually since if later needs to be updated one would need to go through the process of regenerating the serde code for the enum instead of modifying the code directly.

Something along the lines of https://github.com/rust-bitcoin/rust-bitcoin/blob/master/src/blockdata/script.rs#L757-L767 could be done

Actually to me (maybe incorrectly) it seems simpler to only minimally modify the generated code for that exact reason -- one can just regenerate it and patch as needed.

In any case the example you shared is also simpler, since that is a struct and this is an enum.

Hmm yeah I'd prefer not to have these code-generated impls in here :/

I'm not sure where or what you want exactly then?

IMHO renaming the variables for reading them and formatting the code to fit the rest of the file would be enough.

src/util/amount.rs

stevenroose · 2020-10-25T18:59:55Z

#508 might be relevant for the serde_derive issue.

stevenroose · 2020-10-25T19:01:40Z

src/blockdata/script.rs

+    Box<[u8]>);
+
+#[cfg(feature = "schemars")]
+mod schemas {


Perhaps it makes sense to have an special module for all the schemas. Like json_schemas.rs and then refer with schema_with = "::json_schemas::script"? I think that'd be cleaner.

stevenroose · 2020-10-25T19:02:57Z

src/blockdata/transaction.rs

+
+    #[cfg(all(feature = "schemars", feature = "serde"))]
+    #[test]
+    fn schema_correct_outpoint() {


It would be nice if those tests could also fit in the schema module :)

stevenroose · 2020-10-25T19:03:58Z

src/blockdata/transaction.rs

 serde_struct_human_string_impl!(OutPoint, "an OutPoint", txid, vout);

+#[cfg(feature = "schemars")]
+impl JsonSchema for OutPoint {


Those can't be moved into a module, I suppose. Hmm, a bit unfortunate IMO, but I suppose it's fine.

It depends; in this case we need to do a custom impl like this because the representation is weird. If we have per-field representations then we don't need to, but because it becomes one string we need this approach.

Actually in some crates I have seen impl on types within the crate moved into separate modules. I thought that was not possible? Can this block be moved to a file like f.e. json_schemas.rs?

It's cross crate impls that are banned, cross module is fine actually.

more specifically, either the trait or the struct has to be defined in the crate for impl to be allowed.

stevenroose · 2020-10-25T19:05:32Z

src/util/amount.rs

+/// Derived externally using serde_derive
+#[cfg(feature = "serde")]
+#[doc(hidden)]
+#[allow(non_upper_case_globals, unused_attributes, unused_qualifications)]
+const _: () = {
+    #[allow(rust_2018_idioms, clippy::useless_attribute)]
+    extern crate serde as _serde;
+    impl _serde::Serialize for CoinAmount {
+        fn serialize<__S>(&self, __serializer: __S) -> _serde::export::Result<__S::Ok, __S::Error>


Hmm yeah I'd prefer not to have these code-generated impls in here :/

stevenroose · 2020-10-25T19:39:58Z

src/util/amount.rs

+/// Type for dealing with unknown amounts
+#[cfg_attr(feature = "schemars", derive(schemars::JsonSchema))]
+#[derive(Clone, Copy, PartialEq, PartialOrd, Debug)]
+pub enum CoinAmount {


Hmm, is roundtripping for this important? There is already a serde-related module in util::amount that does flexible serialization with sats or btc. It's possible to add a deserializer that can have both, but it's harder to then roundtrip that, so you'd have to chose one to serialize with.

How about something like this: #509

Idk what roundtripping means in this cirumstance. Do you mean the From impls?

The issue with the flexible serialization is that it isn't actually flexible and requires you to annotate with a serializer anywhere you use that type, and then the selection of schemars schema is also not type safe.

A type like CoinAmount can be used in a publicly facing API that can accept either a Bitcoin or a Satoshis amount.

Hmm, yeah I don't know how the different serde deserializers interact with the schema mechanics.

I'm personally not the biggest fan to catering our internal types to the likes of very specific serialization methods, but always building the serialization support around the existing types. That's what has been done with the Amount type. It makes sense internally and there has been provided some infrastructure to use it with serde. Ideally if there are difficulties with schemas, they'd be solved by providing functionality around the Amount and SignedAmount types instead of given a new type.

There's various reasons for this, one is that we want to have a single type per concept, that all serializations can use, not a type per serialization. I can imagine that the CoinAmount doesn't work with other serialization methods like protobuf or so. Also, we shouldn't have types reflect serialization-specific information. Meaning that for the sake of the type itself, it's not useful to keep track on whether the value was derived from serializing 0.01 BTC or 1000000 sat, which the enum seems to do.

What I meant with roundtripping and #509 is that using the flexible deserializer in #509, when re-serializing, the original serialization will be lost.

Ultimately, I think if a specific implementation needs a specific serialization of amounts, it's quite trivial to quickly (1) wrote a serde serialization module around Amount or (2) write a newtype for it and use that for the API of that implementation.

It just seems remarkably easy to, for a programmer, mess up an incoming type as sats or btc. I think an explicitly tagged enum is a much better API for all users as it is completely self-documenting and unambiguous. This isn't just an issue generating the schemas, it's just odd to me that we don't have a type-level way of disambiguatiing the amounts.

Another alternative would be to make Amounts split into AmountsSats and AmountsBtc and then you can use Either<AmountsSats, AmountsBtc> (or result...) to express an api taking either.

But, there is only 1 amount "concept", right? "an amount", aka CAmount in Core.. BTC is just a denomination. And denominations only matter when you're representing the amount. And representation is an application-specific thing, so I think providing flexible tools to help represent our amount type easily in various ways is the way to go instead of creating different types for different presentations.

I think I've figured out the right API here -- what we probably want is to have the type be Amount and use try_from for Amount and serialize_with that defaults to CoinAmount::Sats wrapper. That way the type in the structs is an amount, but we can handle inbound of either.

Then if you need the specific encoding of Amount as a plain u64 or float (without tag), you need to ask for it with an override to the default behavior.

(I also think we should unify Signed and Unsigned amount as CAmount is signed, but that's another question).

In short: at the type level, have a single simple newtype. At the serialization level, have explicit tagged representations with a tagged representation and a fallback mechanism. I'll need to think through the deduction rules and type them up

Hmm yeah I'm not a fan of that approach, but if others disagree, that's fine.

As for Amount and SignedAmount, I think it's useful to have amounts of which you know they cannot be negative.

…ApiAmount the default schema/serialization

JeremyRubin · 2020-11-29T23:35:39Z

I pushed changes which implement the concept based on #501 (comment).

Amount is now default Signed, Amount/UnsignedAmount default serialize/deserialize through a tagged enum; you can still override to have your preferred representation.

stevenroose · 2020-11-30T11:41:18Z

Can you perhaps repeat what exact problem you're trying to solve? This seems like a hugely invasive change and also seems to add a ton of new code.

If the only feature this gets us is "the ability to parse amounts in various formats from JSON", then I'd certainly NACK it. Any API should specify in what format amounts are formatted and can use one of the specified serializers for that (or write his own). For any other freeform user input parsing, we have str::FromStr. If FromStr is not powerful enough, feel free to extend it or perhaps implement a custom parse_amount in your application.

To bridge the gap between FromStr and JSON formatting, #509 implements a serde deserializer using FromStr.

JeremyRubin · 2020-11-30T16:57:37Z

I would suggest this changes irrespective of if you want the JSON API stuff or not. But that is the context in which I first came across the choice made for the Amount type.

The primary issue is that the Amount type is an impedance mismatch for the underlying type used in Bitcoin, which is a signed integer. I think even though it was a poor design choice by Satoshi, it makes sense to not represent that field differently/perform mathematical operations on it differently. For example, why would we care to represent a value > max i64 as an amount?

A second issue is that floating points and integers are not unambiguous. E.g., JSON 1 and 1.0 are valid as both number and integer types (JSON exists with or without JSONSchema mind you). Then it would be possible to -- without indication to a user/api consumer, present a value field where Sats and BTC might ambiguously be accepted and misinterpreted, the worst case of which would lead to destruction/overspending of funds. If there's a concern that various consumers of rust bitcoin APIs might encode values of the same type differently and incompatibly, then it is preferable to use an unambiguous serialization/deserialization with a tagged enum rather than risking incompatibility.

A final point is that suppose we do have a series of floats/btc and ints/sats. We should not make the users perform conversions on their own, it's much preferable to make any conversions happen automatically with rust-bitcoin's correct code.

In my patch it is also possible to not implement Serialize/De at all for Amount/UnsignedAmount therefore forcing the user to always decide between BTC/Sats/Tagged serialization. This would also be OK, I just figured that a tagged representation is a sane default unless you know you want something else.

JeremyRubin · 2021-03-31T18:36:11Z

I know it needs some rebasing, but I would love to find a path forward for integrating this functionality.

As for a context where I use it, you can see https://github.com/sapio-lang/sapio.

I compile bitcoin smart contract plugins into WASM bundles. Being able to output a JSONSchema allows the contract plugins to communicate to other software what parameters the API expects.

Currently I'm maintaining forks of many of the rust crates mainly for this (e.g. https://crates.io/crates/sapio-bitcoin/0.26.0). I can continue doing so, but it's a bit annoying because it means my crates end up incompatible with the main rust-bitcoin crates. Cargo's patching functionality isn't a full fix for this either, because the patches cannot currently be pinned.

If I rebase this (and prs for other crates) can it be merged? Or is this a permanent no-go?

Use for User Generated Forms

In https://github.com/sapio-lang/tux, these JSONSchemas are used to automatically generate user intelligible forms to create contracts.

Use for Dynamic Linking Modules

I'm still working on implementing functionality for processing these schemas and reifying them into types, but this would permit for contract modules to inherit from a common message type. E.g., suppose I define a struct:

#[derive(JsonSchema)
struct HotStorageModule {
    a: PublicKey,
    b: PublicKey
}

I would be able to define

#[derive(JsonSchema)
struct PluginA(HotStorageModule);
#[derive(JsonSchema)
struct PluginB(HotStorageModule);

And a third party module could be defined such that:

struct GiveMeHotStorage {
  module: ImplsInterfaceFor<HotStorageModule>
}

where the ImplsInterfaceFor dynamically loads a contract module.

sgeisler · 2021-03-31T21:43:37Z

I'm fine with it as long as it is feature-gated, I think our general policy around features is that they don't necessarily have to follow our MSRV. While I'd like to reduce our features to keep combinatorial complexity low I think this is probably worth it because you need it for sapio. Currently nobody tests the feature-powerset anyway 😦

I remember the very unfortunate incident in bitcoin_hashes, which was caused by dev-deps not being feature-gateable, so I guess you'd need a way to test it that doesn't break the other tests' MSRV. Maybe the tests can be put into an extra crate which is only tested for stable and nightly (if the same problem exists here)?

JeremyRubin force-pushed the jsonschema branch from ac04f05 to 75a7eac Compare October 14, 2020 01:03

apoelstra reviewed Oct 14, 2020

View reviewed changes

dr-orlovsky reviewed Oct 14, 2020

View reviewed changes

Cargo.toml Show resolved Hide resolved

JeremyRubin mentioned this pull request Oct 14, 2020

Change Cargo.toml serde dev deps #495

Closed

JeremyRubin force-pushed the jsonschema branch 3 times, most recently from e65e493 to c70946d Compare October 15, 2020 18:19

JeremyRubin added 2 commits October 16, 2020 12:46

Pin dev dependencies to at least 1.0

afebcb0

JsonSchema impl for several types

1d0586d

JeremyRubin force-pushed the jsonschema branch from c70946d to 4905595 Compare October 16, 2020 19:48

jeandudey reviewed Oct 16, 2020

View reviewed changes

src/util/amount.rs Outdated Show resolved Hide resolved

Add CoinAmount type

e60c804

JeremyRubin force-pushed the jsonschema branch from 4905595 to e60c804 Compare October 16, 2020 23:15

Bump Version of bitcoin_hashes to have schemars support

4fc3452

stevenroose mentioned this pull request Oct 25, 2020

util::amount: Add a FromStr-based serde deserializer #509

Closed

stevenroose reviewed Oct 25, 2020

View reviewed changes

Rename CoinAmount to ApiAmount; Break up Module of util/amount; Make …

0a7a728

…ApiAmount the default schema/serialization

JeremyRubin closed this Dec 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add JsonSchema Support for Several Types #501

Add JsonSchema Support for Several Types #501

JeremyRubin commented Oct 12, 2020

apoelstra Oct 14, 2020

JeremyRubin Oct 14, 2020

jeandudey Oct 16, 2020

JeremyRubin Oct 16, 2020

stevenroose Oct 25, 2020

JeremyRubin Oct 26, 2020

jeandudey Nov 30, 2020

stevenroose commented Oct 25, 2020

stevenroose Oct 25, 2020

stevenroose Oct 25, 2020

stevenroose Oct 25, 2020

JeremyRubin Oct 26, 2020

stevenroose Oct 31, 2020

JeremyRubin Oct 31, 2020

JeremyRubin Oct 31, 2020

stevenroose Oct 25, 2020

stevenroose Oct 25, 2020

JeremyRubin Oct 26, 2020

stevenroose Oct 31, 2020

JeremyRubin Oct 31, 2020

stevenroose Nov 4, 2020

JeremyRubin Nov 4, 2020

stevenroose Nov 29, 2020

JeremyRubin commented Nov 29, 2020

stevenroose commented Nov 30, 2020

JeremyRubin commented Nov 30, 2020

JeremyRubin commented Mar 31, 2021

sgeisler commented Mar 31, 2021

Add JsonSchema Support for Several Types #501

Add JsonSchema Support for Several Types #501

Conversation

JeremyRubin commented Oct 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevenroose commented Oct 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeremyRubin commented Nov 29, 2020

stevenroose commented Nov 30, 2020

JeremyRubin commented Nov 30, 2020

JeremyRubin commented Mar 31, 2021

Use for User Generated Forms

Use for Dynamic Linking Modules

sgeisler commented Mar 31, 2021