[API] Add remaining endpoints to Poem API backend #2156

banool · 2022-07-22T05:01:35Z

Description

This PR does the following:

Adds all the remaining transaction endpoints, such as submitting and simulating, getting transactions from an account.
Adds the state endpoints, including the table endpoints.
Separates our docs into v0 and v1.
Separates our tests into v0 and v1.
Separates our golden files into v0 and v1.
Updates the common testing code to handle both of these API versions where appropriate.
Adds middleware for confirming that the payload for POST requests is not too large.
Improves error handling so all errors are represented as a JSON representation of the internal AptosError.
Removes a bunch of loose todos in favor of freshly made issues.
Updates the tags for the endpoints.

In the interest of moving fast I haven't split the commits up super nicely, but I can do so if this is too hard to review.

Once this PR lands, the new API will be pretty much ready to go, pending these issues: https://github.com/aptos-labs/aptos-core/issues?q=is%3Aissue+is%3Aopen+%5BAPI%5D%5Bv1%5D (many of which are nice-to-haves).

There are other things that would be nice to solve too, pending work with the framework:

Note: Currently the PR uses Poem from git directly, but that's just because I'm waiting for a committed change to make its way to crates.io (probably a day or two).

Note: I need to clean up some other tests I think, based on the output of cargo nextest run --package smoke-test --test-threads 8.

Test Plan

Run a node:

cargo run -p aptos -- node run-local-testnet

Sample queries with curl:

curl localhost:8080/v1/transactions/by_hash/0xebef435ff57c3e0444fad14587ecdaa7fafea704a3e40d95ba1bb5930773ec53 | jq .hash
"0xebef435ff57c3e0444fad14587ecdaa7fafea704a3e40d95ba1bb5930773ec53"

curl localhost:8080/v1/transactions/by_version/443 | jq .version
443

Generating goldenfiles for the new API:

REGENERATE_GOLDENFILES=1 cargo test -- v1

Confirming that the v0 API tests pass:

cargo test -- v0

There are minor changes to the v0 API, e.g. returning epoch as a U64 instead of u64, or changes to how we represent type tags. I have tested that this doesn't break any of our clients / just add extra code so those changes don't manifest in the v0 API, but I still need to do more testing (particularly for TS).

This change is

rajkaramchedu · 2022-07-28T18:40:17Z

I had disabled the plugin-generated API docs and instead linked to the Stoplight version. To turn it back to using the new version, just change the links from fullnode... to /rest-api on these two lines, shown in this earlier PR: https://github.com/aptos-labs/aptos-core/pull/2176/files

gregnazario

So I don't see any big red flags, but my main concern is the number of TODOs and issues attached here. Are there any major blockers / behavior differences here from the original API?

gregnazario · 2022-07-28T17:53:11Z

crates/aptos-openapi/Cargo.toml

+poem = { git = "https://github.com/poem-web/poem" }
+poem-openapi = { git = "https://github.com/poem-web/poem" }
 serde = { version = "1.0.137", default-features = false }


Why do we use the direct git rather than the releases?

I mention this in the summary, I'm waiting for a recently landed change to make its way to crates.io.

gregnazario · 2022-07-28T19:42:53Z

api/doc/v1/README.md

@@ -0,0 +1,3 @@
+# Aptos Node API v1
+
+todo


:(. Please put that this is in development / experimental or something like that.

I have another PR on top of this that adds all the docs.

gregnazario · 2022-07-28T19:44:48Z

api/src/poem_backend/accept_type.rs

@@ -20,6 +22,7 @@ pub fn parse_accept<E: BadRequestError>(accept: &Accept) -> Result<AcceptType, E
            "application/x-bcs" => return Ok(AcceptType::Bcs),


Note we should use constants for these instead that we use elsewhere.

Agreed. Beyond just constants, we should unify them: #2275.

gregnazario · 2022-07-28T19:45:16Z

api/src/blocks.rs

@@ -43,7 +43,7 @@ impl Block {
        if ledger_version > latest_ledger_info.version() {
            return Err(Error::not_found(
                "ledger",
-                TransactionId::Version(ledger_version),
+                TransactionId::Version(U64::from(ledger_version)),


Thanks for fixing this, considering it wouldn't parse consistently.

gregnazario · 2022-07-28T19:46:36Z

api/types/src/wrappers.rs

+// Currently it is not possible to deserialize certain MoveTypes, such as
+// generic type params. In those cases, we give up on parsing them as


Why can't it be parsed with generic type params? I ended up doing this myself for the type, just deserializing the actual underlying struct might not work.

For starters, the underlying function within the move create just doesn't handle generic type params at all. For this to work properly, I'd need to be able to inject additional parsing capabilities into the function so when it eventually parses down to the generic type param token, it knows what to do with it. Even this isn't enough because the enum they use doesn't have this as an option. So if we wanted to support it we'd have to add rewrite a lot of stuff ourselves (unless we got this support into move directly, but Victor says it doesn't really make sense there because it's only meant for concrete types).

gregnazario · 2022-07-28T19:50:01Z

api/types/src/transaction.rs

@@ -31,7 +31,8 @@ use std::{
 };

 // TODO: Add read_only / write_only (and their all variants) where appropriate.
-// TODO: Explore whether we should use discriminator_name, see https://github.com/poem-web/poem/issues/329.
+// TODO: Investigate the use of discriminator_name, see https://github.com/poem-web/poem/issues/329.
+// TODO: See https://github.com/poem-web/poem/issues/347 re mapping stuff. UPDATE: Wait for 2.0.7 to be released.


What's the timeline on that? And what does it block?

The mapping stuff should be out tomorrow. As for the discriminator stuff where we remove intermediates, unknown. It doesn't block anything, it just adds some extra intermediate types that bloat the spec a bit.

gregnazario · 2022-07-28T19:52:38Z

api/src/tests/v1/invalid_post_request_test.rs

+        "sequence_number": "0",
+        "gas_unit_price": "0",
+        "max_gas_amount": "1000000",
+        "gas_currency_code": "XUS",


If we can get rid of XUS we should.

Yeah we do actually, I don't think this test is enabled right now (for either v0 or v1). But in v1 there is no gas currency code field (not by my own choice, just by using the types properly).

gregnazario · 2022-07-28T19:53:42Z

api/src/tests/v0/string_resource_test.rs

@@ -38,7 +39,8 @@ async fn test_renders_move_acsii_string_into_utf8_string() {
    context
        .api_execute_script_function(
            &mut account,
-            "Message::set_message",
+            "Message",


this module should be snake case, interesting that it runs.

gregnazario · 2022-07-28T21:14:50Z

api/src/poem_backend/error_converter.rs

+        response
+            .headers_mut()
+            .insert(CONTENT_TYPE, HeaderValue::from_static(JSON));


Errors are always JSON?

Yeah, I make sure of that.

gregnazario · 2022-07-28T21:15:18Z

api/src/poem_backend/page.rs

@@ -22,7 +22,7 @@ impl Page {
        let start = self.start.unwrap_or(default);
        if start > max {
            return Err(E::bad_request_str(&format!(
-                "Given start value ({}) is too large, it must be < {}",
+                "Given start value ({}) is higher than the highest ledger version, it must be < {}",


*current ledger version

banool · 2022-07-28T22:43:48Z

There are certainly a lot of todos, but almost all of them are nice-to-haves. I'm currently working on those that aren't nice to haves. There are small functionality differences between the two APIs (generally the new API has more structure in its requests / responses). I've almost got a TS client working with the new API, beyond just the Rust tests passing.

github-actions · 2022-07-29T16:02:10Z

✅ Forge test success on `ab72d73204a00c8a309d692a56ce2b31118a34c9`

all up : 4815 TPS, 2284 ms latency, 3150 ms p99 latency,no expired txns

* [API] Fix up tags for each endpoint * [API] Add POST /transactions * [API] Add /transactions/encode_submission * [API] Add get_transaction for single transaction * [API] Add /transactions/simulate to Poem backend * [API] Add /accounts/{address}/transactions to Poem backend * [API] Add /accounts/{address}/resource/{resource_type} to Poem backend * [API] Add /accounts/{address}/module/{resource_type} to Poem backend * [API] Add /table/{table_handle}/item to Poem backend * [API] Use U64 and U128 at all endpoints * [API] Fix up spec generation for some types * [API][v1] Enhance OpenAPI spec "newtypes"

banool force-pushed the banool/poem_api_remaining_endpoints branch 2 times, most recently from e5b67be to ecffe6e Compare July 22, 2022 05:17

banool force-pushed the banool/complete_poem_api branch from 64c8c08 to ef0a578 Compare July 22, 2022 14:40

banool force-pushed the banool/poem_api_remaining_endpoints branch from ecffe6e to 60027e6 Compare July 24, 2022 01:42

banool force-pushed the banool/complete_poem_api branch 2 times, most recently from f3b8f20 to b6576de Compare July 26, 2022 00:35

Base automatically changed from banool/complete_poem_api to main July 26, 2022 01:12

banool force-pushed the banool/poem_api_remaining_endpoints branch 8 times, most recently from 13c9275 to 6a48105 Compare July 28, 2022 07:40

banool marked this pull request as ready for review July 28, 2022 07:43

banool requested review from bowenyang007, gregnazario and jjleng July 28, 2022 18:03

banool force-pushed the banool/poem_api_remaining_endpoints branch from 6a48105 to 65bb459 Compare July 28, 2022 18:31

banool requested review from davidiw and rajkaramchedu as code owners July 28, 2022 18:31

banool force-pushed the banool/poem_api_remaining_endpoints branch from 65bb459 to fb81b0c Compare July 28, 2022 20:40

gregnazario reviewed Jul 28, 2022

View reviewed changes

banool force-pushed the banool/poem_api_remaining_endpoints branch from dd5b50b to 5200f1d Compare July 29, 2022 01:05

gregnazario approved these changes Jul 29, 2022

View reviewed changes

banool added 2 commits July 29, 2022 07:58

[API] Fix up tags for each endpoint

1de1979

[API] Add POST /transactions

a990570

banool added 9 commits July 29, 2022 07:58

[API] Add /transactions/encode_submission

a2800f9

[API] Add get_transaction for single transaction

719fce9

[API] Add /transactions/simulate to Poem backend

4bcaca1

[API] Add /accounts/{address}/transactions to Poem backend

bc9d7c8

[API] Add /accounts/{address}/resource/{resource_type} to Poem backend

f72f710

[API] Add /accounts/{address}/module/{resource_type} to Poem backend

6652014

[API] Add /table/{table_handle}/item to Poem backend

f166204

[API] Use U64 and U128 at all endpoints

1dee62f

[API] Fix up spec generation for some types

1be6703

banool force-pushed the banool/poem_api_remaining_endpoints branch from 5200f1d to 7d902a0 Compare July 29, 2022 15:05

banool enabled auto-merge (squash) July 29, 2022 15:05

[API][v1] Enhance OpenAPI spec "newtypes"

ab72d73

banool force-pushed the banool/poem_api_remaining_endpoints branch from 7d902a0 to ab72d73 Compare July 29, 2022 15:26

banool merged commit ff05779 into main Jul 29, 2022

banool deleted the banool/poem_api_remaining_endpoints branch July 29, 2022 16:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[API] Add remaining endpoints to Poem API backend #2156

[API] Add remaining endpoints to Poem API backend #2156

banool commented Jul 22, 2022 •

edited

Loading

rajkaramchedu commented Jul 28, 2022

gregnazario left a comment

gregnazario Jul 28, 2022

banool Jul 28, 2022

gregnazario Jul 28, 2022

banool Jul 28, 2022

gregnazario Jul 28, 2022

banool Jul 28, 2022

gregnazario Jul 28, 2022

gregnazario Jul 28, 2022

banool Jul 28, 2022

gregnazario Jul 28, 2022

banool Jul 28, 2022

gregnazario Jul 28, 2022

banool Jul 28, 2022

gregnazario Jul 28, 2022

gregnazario Jul 28, 2022

banool Jul 28, 2022

gregnazario Jul 28, 2022

banool commented Jul 28, 2022

github-actions bot commented Jul 29, 2022

		@@ -20,6 +22,7 @@ pub fn parse_accept<E: BadRequestError>(accept: &Accept) -> Result<AcceptType, E
		"application/x-bcs" => return Ok(AcceptType::Bcs),

		// Currently it is not possible to deserialize certain MoveTypes, such as
		// generic type params. In those cases, we give up on parsing them as

[API] Add remaining endpoints to Poem API backend #2156

[API] Add remaining endpoints to Poem API backend #2156

Conversation

banool commented Jul 22, 2022 • edited Loading

Description

Test Plan

rajkaramchedu commented Jul 28, 2022

gregnazario 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

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

banool commented Jul 28, 2022

github-actions bot commented Jul 29, 2022

✅ Forge test success on ab72d73204a00c8a309d692a56ce2b31118a34c9

banool commented Jul 22, 2022 •

edited

Loading

✅ Forge test success on `ab72d73204a00c8a309d692a56ce2b31118a34c9`