Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TS SDK] Support contentType in request #8977

Merged
merged 2 commits into from
Jul 6, 2023
Merged

Conversation

0xmaayan
Copy link
Contributor

@0xmaayan 0xmaayan commented Jul 6, 2023

Description

Current implementation overrides existing 'content-type':'application/x.aptos.signed_transaction+bcs' header when passing in custom headers.
This PR adds contentType support that when submitting a transaction, the expected content type would be set.
more context here https://aptos-org.slack.com/archives/C04D0LSBVJN/p1688610023758839

Test Plan

tests are passing
added specific test https://github.com/aptos-labs/aptos-core/compare/add_mediaType_to_request?expand=1#diff-9187cd50043dfc9d3829abdb7a919d2497b633279daa100db09194b2d6509366R45

@0xmaayan 0xmaayan force-pushed the add_mediaType_to_request branch from 1917b23 to bb5c716 Compare July 6, 2023 15:19
@0xmaayan 0xmaayan changed the title [TSSDK] Support mediaType in request [TS SDK] Support mediaType in request Jul 6, 2023
expect(true).toBe(false);
}
},
longTestTimeout,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious, why do we include a longTestTimeout on every test now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is flaky on how much time it takes for the test to complete, if you notice we use it on most of the tests we just didn't have it on this file

endpoint: "transactions",
body: bcsTxn,
originMethod: "test request includes all headers",
mediaType: "application/x.aptos.signed_transaction+bcs",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, so when the request is a bcs serialized txn, the content type need to be set to application/x.aptos.signed_transaction+bcs? Hmm not sure if I seen this in Python nor Rust sdk

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting.... we should have it, without it API explodes lol

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ohhh nice find!

@@ -26,12 +26,14 @@ async function axiosRequest<Request, Response>(
url: string,
method: "GET" | "POST",
body?: Request,
mediaType?: string,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's call it contentType instead, since that's the actual header we're using.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

ecosystem/typescript/sdk/src/client/types.ts Outdated Show resolved Hide resolved
@0xmaayan 0xmaayan force-pushed the add_mediaType_to_request branch from bb5c716 to 9666466 Compare July 6, 2023 19:13
@0xmaayan 0xmaayan changed the title [TS SDK] Support mediaType in request [TS SDK] Support contentType in request Jul 6, 2023
@0xmaayan 0xmaayan force-pushed the add_mediaType_to_request branch from 9666466 to 96f08ae Compare July 6, 2023 19:14
@0xmaayan 0xmaayan enabled auto-merge (squash) July 6, 2023 19:17
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

github-actions bot commented Jul 6, 2023

✅ Forge suite compat success on aptos-node-v1.4.4 ==> b95cfaebcb8b44c14ad86a5772384518aa58abee

Compatibility test results for aptos-node-v1.4.4 ==> b95cfaebcb8b44c14ad86a5772384518aa58abee (PR)
1. Check liveness of validators at old version: aptos-node-v1.4.4
compatibility::simple-validator-upgrade::liveness-check : committed: 6980 txn/s, latency: 4809 ms, (p50: 4700 ms, p90: 6600 ms, p99: 8300 ms), latency samples: 237320
2. Upgrading first Validator to new version: b95cfaebcb8b44c14ad86a5772384518aa58abee
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 4567 txn/s, latency: 7132 ms, (p50: 8300 ms, p90: 9500 ms, p99: 10400 ms), latency samples: 169000
3. Upgrading rest of first batch to new version: b95cfaebcb8b44c14ad86a5772384518aa58abee
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 3887 txn/s, latency: 8011 ms, (p50: 8700 ms, p90: 9700 ms, p99: 10200 ms), latency samples: 151600
4. upgrading second batch to new version: b95cfaebcb8b44c14ad86a5772384518aa58abee
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 5945 txn/s, latency: 5333 ms, (p50: 5200 ms, p90: 7500 ms, p99: 9800 ms), latency samples: 214020
5. check swarm health
Compatibility test for aptos-node-v1.4.4 ==> b95cfaebcb8b44c14ad86a5772384518aa58abee passed
Test Ok

@github-actions
Copy link
Contributor

github-actions bot commented Jul 6, 2023

✅ Forge suite realistic_env_max_load success on b95cfaebcb8b44c14ad86a5772384518aa58abee

two traffics test: inner traffic : committed: 5746 txn/s, latency: 6793 ms, (p50: 6300 ms, p90: 7800 ms, p99: 24700 ms), latency samples: 2493900
two traffics test : committed: 100 txn/s, latency: 2812 ms, (p50: 2500 ms, p90: 3500 ms, p99: 8600 ms), latency samples: 1860
Max round gap was 2 [limit 4] at version 1125382. Max no progress secs was 5.223344 [limit 10] at version 1125382.
Test Ok

@github-actions
Copy link
Contributor

github-actions bot commented Jul 6, 2023

✅ Forge suite framework_upgrade success on aptos-node-v1.4.4 ==> b95cfaebcb8b44c14ad86a5772384518aa58abee

Compatibility test results for aptos-node-v1.4.4 ==> b95cfaebcb8b44c14ad86a5772384518aa58abee (PR)
Upgrade the nodes to version: b95cfaebcb8b44c14ad86a5772384518aa58abee
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 3126 txn/s, latency: 7005 ms, (p50: 7500 ms, p90: 9500 ms, p99: 10100 ms), latency samples: 171940
5. check swarm health
Compatibility test for aptos-node-v1.4.4 ==> b95cfaebcb8b44c14ad86a5772384518aa58abee passed
Test Ok

@0xmaayan 0xmaayan merged commit c9b4e4a into main Jul 6, 2023
@0xmaayan 0xmaayan deleted the add_mediaType_to_request branch July 6, 2023 19:56
gedigi pushed a commit that referenced this pull request Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants