Add preview, submit tx and tx stream APIs in Rust #95

LukasGasior1 · 2022-08-24T18:40:46Z

No description provided.

state-manager/core-api-server/core-api-spec.yaml

krzlabrdx · 2022-08-26T15:22:23Z

state-manager/core-api-server/core-api-spec.yaml

+          content:
+            application/json:
+              schema:
+                "$ref": "#/components/schemas/ErrorResponse"
        '500':


IMHO it's quite pointless to specify 500 responses, just focus on responses returned by application that serve business purposes (200 and 400 in this case).
500, 501, 502, 503, 401, 402, 100, ... - if customer to our API is not expected to face them not to mention handle them what's the point of documenting them? BTW we can't guarantee response payload format for 500 errors :)

Agreed, I don't see much point in specifying it either. As a side note, I'll also get rid of the leftover polymorphic error types (InvalidTransactionError, PreviewError etc) - it isn't supported by the Rust generator.

Hmh, I take that back, it seems that with the Rust openapi gen it's required to define the 500 error responses if we want to return any meaningful messages for server errors. Otherwise we can't return a custom message (it's hardcoded to a generic "An internal error occurred").

Overall, there are 4 categories of error responses:

a request that doesn't match the schema: 400 + txt error message (no json) - handled by the generated server

client error (or any error that can loosely fit into this category, e.g. "mempool full" error) - 400 + json error (code, message) - corresponds to Ok(SomeResponse::ClientError)

expected/handled server error (e.g. state manager lock error) - 500 + json error - corresponds to Ok(SomeResponse::ServerError)

unhandled server error - 500 + generic txt error message - corresponds to Err(ApiError(...)) - we shouldn't really use it

It could be that "unhandled server error - 500 + generic txt error message" is also returned by the generated server in some cases

@KrzysztofLabus-RDX - just for context, the Core API (like the Olympia Gateway) was designed to follow "RPC over HTTP" so everything which isn't a 200 is a 500; with a separate internal code.

I'd be potentially happy to change this for Babylon, but it'd need a wider conversation.

@dhedey Ah, right! Forget the question/suggestion then :)

"everything which isn't a 200 is a 500;" is that really the case? I thought that'd be "everything which isn't a 200 is either a 400 or a 500;"

Honestly, I'm happy to use proper HTTP response codes. But paging @talekhinezh in case he wants to argue for keeping 500 for everything

krzlabrdx · 2022-08-26T15:26:34Z

state-manager/core-api-server/core-api-spec.yaml

+          items:
+            type: string
+        error_message:
+          type: string


Could we please define optional (non-required) properties as nullable? There's long reasoning behind that: https://radixdlt.atlassian.net/browse/NG-64?jql=text%20~%20%22nullable%20json%22 tl;dr: changes nothing in practice but makes it possible to define proper backing types (think: POJOs) in virtually any sane language (Java, C#, PHP etc.)

yup, can do

This, on the other hand, kind-of breaks the Rust generation :D Non-required nullable fields become Option<Nullable<T>> which isn't ideal, but I guess we can live with that.

btw, did you just call PHP a sane language!?

I believe you're using this OAS Generator to automatically generate backing types, right? So it looks like it's capable of producing proper backing types with use of Ruts' Option. I'll take my request back. What you've been doing was just right. I'll simply try to fix C# OAS Generator generator to produce nullable resource where appropriate. Ideally we could improve it to the point where C# generator also uses Optional<T> where T is nullable or non-nullable type but that would be more difficult due to various reasons. CC @dhedey

EDIT: Yeah, I've called PHP sane language, shame on me. :D

If we're not supporting nullable types in the Core API, then, yes my PR here documents two distinct issues with the handling of required/not-required types in the C# generation: OpenAPITools/openapi-generator#11607 - and fixes issue (2) - but hasn't been merged. I think we're talking about issue (1) here.

It's quite important in the C# that both (1) and (2) are fixed in the generated models - or at least it was for Olympia.

I'd possibly recommend either picking up the PR, and/or fixing these in template file overrides locally - see eg https://github.com/radixdlt/radixdlt-python-clients/tree/main/regeneration and https://openapi-generator.tech/docs/templating

Ok, removed "nullable"

dhedey

Looks good! A few suggestions for minor tweaks. Excited to see this speed us up compared to having this in Java.

Is the plan to rename /state-manager/ to eg /rust (or something) in a small PR at some point? :)

dhedey · 2022-08-29T10:48:01Z

state-manager/core-api-server/core-api-spec.yaml

+        - notary_signature
+      properties:
+        hash:
+          type: string


In Olympia, Josh designed the API to return explicit wrapped "identifier" objects, I'd be happy to ditch these though - but just wanted to check this was an intentional direction?

We could also make these a $ref to a string type with docs, in order to de-dupe any docs we want to add :)

It was more accidental than intentional, I kind of like the idea of having those defined as separate types

dhedey · 2022-08-29T10:48:57Z

state-manager/core-api-server/core-api-spec.yaml

+        notary_as_signatory:
+          type: boolean
+        cost_unit_limit:
+          type: string


Can we document what these mean / represent?

will do 👍

dhedey · 2022-08-29T11:04:42Z

state-manager/core-api-server/core-api-spec.yaml

@@ -363,16 +457,13 @@ components:
          type: string
        cost_unit_limit:
          description: A limit of cost units available for execution


For these three options, can we mention in the documentation that these are integers?

I know in Olympia for integers, we used format: integer etc, and reserved strings for big integers, or longs where we're likely to go about 2^53 (cf https://stackoverflow.com/users/596689/tom-christie ).

But also potentially happy to just use strings if we feel this is more consistent; but I'd like us to ensure we are consistent, and to document this somewhere. (Inline in each explanation; and the general approach and justification in the overall API description).

good point, wdyt about adding custom types (and use $ref) for UInt*?

ah, but that might not work well with descriptions

Yeah - sadly this doesn't work so well with descriptions :( - otherwise I love it.

dhedey · 2022-08-29T11:13:03Z

state-manager/core-api-server/src/core_api/transactions.rs

+        .lock()
+        .map_err(|_| submit_server_error("Internal server error (state manager lock)"))?;
+
+    let result = locked_state_manager.mempool.add_transaction(transaction);


It's worth us flagging to @talekhinezh that this doesn't go through any code in the Java Ledger/StateManager, suggesting anything like metrics / locking / error handling should move down into the Rust mempool eventually 👍

Yup this is fine.

dhedey · 2022-08-29T11:14:07Z

state-manager/core-api-server/src/core_api/transactions.rs

+    TransactionSubmitPostResponse::ServerError(ErrorResponse::new(500, message.to_string()))
+}
+
+pub(crate) fn handle_transactions(


Can we name this slightly more clearly? eg handle_get_committed_transactions

dhedey · 2022-08-29T11:16:43Z

state-manager/core-api-server/src/core_api/transactions.rs

+    let mut txns = vec![];
+    let mut state_version = initial_state_version;
+    while state_version <= up_to_state_version_inclusive {
+        if let Some(next_tid) = locked_state_manager.proof_store.get_tid(state_version) {


When would we expect state version to be missing from the state store?

What do you think about us (A) reading some "max state version", (B) capping up_to_state_version_inclusive to the max, (C) raising error for any state version missing, (D) returning max state version in the response?

(Maybe in a follow up PR so that we can merge for Krzysztof)

I think we're currently missing a tx for state version 0, but this can also be handled; will do in a separate PR

Transactions map to resultant state versions, so I think it's right that there is no tranasction mapping to resultant state version 0.

The Genesis transaction would map 0 to 1, so be at resultant state version 1 👍

yup, makes sense, done

…preview

dhedey

Nice! Let's get this merged. Happy for any fixes to follow

dhedey · 2022-08-30T11:16:30Z

state-manager/core-api-server/core-api-spec.yaml

@@ -164,8 +165,10 @@ components:
          type: array
          items:
            type: string
+          description: The engine return data (only present if status is succeeded)


Is this true? I imagine if failed, they'd still be output, because there's still state changes and a commit?

we might need a better naming :) the state diff is still there, but not the output (as in function return value):

pub enum TransactionOutcome { Success(Vec<Vec<u8>>), Failure(RuntimeError), }

Ahh right, yes. Sorry, got you. This is the "output", which is really the "transaction executor log" I think? I'm not really sure what it is to be honest xD.

Yulong and I were using the term "Outcome" for Success/Failure :)

sonarqubecloud · 2022-08-30T16:04:03Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

LukasGasior1 added 3 commits August 24, 2022 20:39

Add preview, submit tx and tx stream APIs in Rust

7f566be

Merge branch 'feature/openapi-rust' into feature/openapi-rust-preview

5716db3

Merge branch 'feature/openapi-rust' into feature/openapi-rust-preview

96e2337

LukasGasior1 changed the base branch from feature/openapi-rust to main August 25, 2022 16:24

LukasGasior1 changed the base branch from main to feature/openapi-rust August 25, 2022 16:24

LukasGasior1 mentioned this pull request Aug 26, 2022

Add core API server in Rust #91

Merged

Base automatically changed from feature/openapi-rust to main August 26, 2022 12:58

LukasGasior1 marked this pull request as ready for review August 26, 2022 12:59

LukasGasior1 added 2 commits August 26, 2022 14:59

Merge branch 'feature/openapi-rust' into feature/openapi-rust-preview

ace3d04

Network configuration: return only network hrp suffix

61ab66d

krzlabrdx suggested changes Aug 26, 2022

View reviewed changes

dhedey approved these changes Aug 29, 2022

View reviewed changes

LukasGasior1 added 5 commits August 29, 2022 15:42

Better error handling

0db9424

Merge remote-tracking branch 'origin/main' into feature/openapi-rust-…

1873f54

…preview

Remove nullable in api specs

024e4a9

Improve core api descriptions

764a084

Return error on missing txn state versions

4a3eb2e

LukasGasior1 force-pushed the feature/openapi-rust-preview branch from 0f3afcd to 4a3eb2e Compare August 29, 2022 17:52

Rename state-manager to core-rust

1f9e26b

krzlabrdx approved these changes Aug 30, 2022

View reviewed changes

dhedey approved these changes Aug 30, 2022

View reviewed changes

LukasGasior1 merged commit e25be39 into main Aug 30, 2022

LukasGasior1 deleted the feature/openapi-rust-preview branch August 30, 2022 11:18

Add preview, submit tx and tx stream APIs in Rust #95

Add preview, submit tx and tx stream APIs in Rust #95

Conversation

LukasGasior1 commented Aug 24, 2022

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

dhedey Aug 29, 2022 • edited Loading

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

LukasGasior1 Aug 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krzlabrdx Aug 29, 2022 • edited Loading

Choose a reason for hiding this comment

dhedey Aug 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhedey left a 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

LukasGasior1 Aug 29, 2022 • edited Loading

Choose a reason for hiding this comment

LukasGasior1 Aug 29, 2022 • edited Loading

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

dhedey left a 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

sonarqubecloud bot commented Aug 30, 2022

dhedey Aug 29, 2022 •

edited

Loading

LukasGasior1 Aug 26, 2022 •

edited

Loading

krzlabrdx Aug 29, 2022 •

edited

Loading

dhedey Aug 29, 2022 •

edited

Loading

LukasGasior1 Aug 29, 2022 •

edited

Loading

LukasGasior1 Aug 29, 2022 •

edited

Loading