[aptos-sdk-builder] Rebrand and re-emerge transaction-builder-generator #2151

davidiw · 2022-07-22T02:22:15Z

Renamed it, transaction-builder-generator is a long awkward name that doesn't align with what the project actually does. It is a tool to generate SDKs.
encode_ prefix on functions is unnecessarily verbose
Updated to a new version of downstream dependencies that reveal better errors on missing types
Added the missing type that was breaking generics

This change is

wrwg · 2022-07-22T02:26:20Z

Quick question: does this now also work again? Because it crashed after the module renaming and was deactivated. (Both for cached and framework run.)

davidiw · 2022-07-22T02:35:11Z

Quick question: does this now also work again? Because it crashed after the module renaming and was deactivated. (Both for cached and framework run.)

Yup, I fixed it. First had to improve the upstream library to determine why it was exploding and then had to fix this. Apparently it was missing "TypeTag", which is sad because that was what we were looking to generate but failing to do so.

Mind you, it passed some tests, so I'm curious to see if it passes the litany of tests we have. 🤞

wrwg · 2022-07-22T02:58:17Z

Quick question: does this now also work again? Because it crashed after the module renaming and was deactivated. (Both for cached and framework run.)

Yup, I fixed it. First had to improve the upstream library to determine why it was exploding and then had to fix this. Apparently it was missing "TypeTag", which is sad because that was what we were looking to generate but failing to do so.

Mind you, it passed some tests, so I'm curious to see if it passes the litany of tests we have. 🤞

Great! Can we keep the generated files checked in, and have a little shell to regenerate those guys? I don't want to go to the same ordeal again. In general there are two issues with the previous setup:

It is not friendly to IDEs as you can't see those generated functions in completion no do goto-definition. Most of us are using IDEs AFAICT.
It introduces a dependency of the build process from the thing which is being built. This is also called the "bootstrap problem" in compiler construction. If execution of the transaction builder gen crashes, then you can't build existing tests elsewhere, which in turn blocks you from debugging why it crashes.

davidiw · 2022-07-22T03:10:17Z

This is a tricky situation, because on the one hand, you're right it is a pain in the ass. On the other hand, it builds uncomfortable nuance into the codebase -- that is, I must re-run the builder and copy the file over. Let me explore a couple solutions that might ... automate this.

wrwg · 2022-07-22T03:25:08Z

This is a tricky situation, because on the one hand, you're right it is a pain in the ass. On the other hand, it builds uncomfortable nuance into the codebase -- that is, I must re-run the builder and copy the file over. Let me explore a couple solutions that might ... automate this.

The most straightforward way is to build a test for this. We are doing this in the Move repo for generated artifacts like docs. See the cargo test target of the move-stdlib.

Practically there is little need to automate this since only a few people will run into the need to re-run this, and they should know what they are doing/need to do.

wrwg · 2022-07-22T03:27:06Z

I must re-run the builder and copy the file over.

No copy should be needed. This should be done in Rust. (And also the test logic -- its like a baseline test.)

zekun000 · 2022-07-22T06:28:10Z

aptos-move/aptos-sdk-builder/Cargo.toml

@@ -14,8 +14,8 @@ anyhow = "1.0.57"
 bcs = "0.1.3"
 heck = "0.3.2"
 regex = "1.5.5"
-serde-generate = { git = "https://github.com/aptos-labs/serde-reflection" }
-serde-reflection = { git = "https://github.com/aptos-labs/serde-reflection" }
+serde-generate = { git = "https://github.com/aptos-labs/serde-reflection", rev = "9ba2437dc079515f5a1797b8536b0ea7685678d0" }


what is this?

The upstream serde reflection has issues, we forked a while ago. Once we get into a good place, I'll chat with Mathieu about potentially using his.

this is not upstream though? isn’t this our fork?

movekevin · 2022-07-22T06:45:51Z

aptos-move/framework/cached-packages/build.rs

@@ -14,7 +14,7 @@ fn main() {
        no_check_layout_compatibility: false,
        no_build_docs: false,
        with_diagram: false,
-        no_script_builder: true, // TODO: consider bringing this back
+        no_script_builder: false,


what does this do?

turns generation of the script builder back on.

movekevin · 2022-07-22T06:47:34Z

aptos-move/e2e-testsuite/src/tests/on_chain_configs.rs

@@ -25,7 +25,7 @@ fn initial_aptos_version() {
        println!("@@@@@@@@@@@@ account {:?}", account);


Oh crap this looks like mine. Can you please remove it? I'm surprised lint didn't catch this.

movekevin · 2022-07-22T06:47:41Z

aptos-move/e2e-testsuite/src/tests/on_chain_configs.rs

@@ -25,7 +25,7 @@ fn initial_aptos_version() {
        println!("@@@@@@@@@@@@ account {:?}", account);
        let txn = account
            .transaction()
-            .payload(aptos_stdlib::encode_version_set_version(test_env.version_number + 1))
+            .payload(aptos_stdlib::version_set_version(test_env.version_number + 1))
            .sequence_number(test_env.dr_sequence_number)
            .sign();
        println!("@@@@@@@@@@@@ txn {:?}", txn);


* Upgraded to a newer version that spit out more clean errors * Fixed the missing TypeTag * Remove encode and decode prefixes from functions * Adjusted to new casing of module names * Remove blocklist of unsupported functions

davidiw · 2022-07-22T16:01:42Z

PR should be ready for review and hopefully landing, so I can focus on the next big challenge :)

Now that the former transaction-generator-builder is healthy, I've reintroduced it. I've removed the unnecessary encode_ prefix and updated the functions appropriately.

For move arguments, VecBytes and Vec<Vec<u8>> should be interchangeable. The former is more natural, since theoretically, we can have nested vectors of other types.

areshand · 2022-07-22T16:41:47Z

aptos-move/framework/cached-packages/src/aptos_stdlib.rs

@@ -3,29 +3,15 @@

 #![allow(unused_imports)]


why is not this called aptos_stdlib_builder?

hahaha, probably should rename these to just lib and no builder.

areshand · 2022-07-22T16:48:50Z

aptos-move/aptos-sdk-builder/src/common.rs

@@ -34,7 +34,7 @@ fn quote_type_as_format(type_tag: &TypeTag) -> Format {
            U8 => Format::Bytes,
            Vector(type_tag) => {
                if type_tag.as_ref() == &U8 {
-                    Format::TypeName("VecBytes".into())


we may want to keep consistent with the types in move repo. If we need to change anything on the parsing of VecBytes in move repo, it would be much easier to use existing types.

VecBytes are a hack as far as I can tell. We should probably work with the team to remove them. Also move doesn't really have a notion of "transaction agruments".

github-actions · 2022-07-22T16:50:42Z

✅ Forge test success

Forge is land-blocking

all up : 5578 TPS, 3063 ms latency, 6150 ms p99 latency,no expired txns

github-actions · 2022-07-22T22:40:19Z

✅ Forge test success

Forge is land-blocking

all up : 5669 TPS, 3009 ms latency, 5450 ms p99 latency,no expired txns

SDK files are useful for ides and general code readability. This stores them in the repo. They will automatically be rebuilt each time the user builds cached-framework to make it easier to keep them up to date without the user needing to explicitly figure this out.

github-actions · 2022-07-23T00:20:37Z

✅ Forge test success

Forge is land-blocking

all up : 5883 TPS, 2889 ms latency, 5300 ms p99 latency,no expired txns

davidiw requested review from wrwg, zekun000 and movekevin July 22, 2022 02:22

davidiw force-pushed the move branch from de0a0a6 to ccee48c Compare July 22, 2022 02:27

davidiw force-pushed the move branch from ccee48c to f0648bc Compare July 22, 2022 03:07

davidiw closed this Jul 22, 2022

davidiw reopened this Jul 22, 2022

zekun000 reviewed Jul 22, 2022

View reviewed changes

movekevin reviewed Jul 22, 2022

View reviewed changes

davidiw requested a review from gregnazario July 22, 2022 15:32

davidiw force-pushed the move branch from f0648bc to 7a57d34 Compare July 22, 2022 16:00

davidiw force-pushed the move branch from 8a8993b to aa33ece Compare July 22, 2022 16:01

wrwg approved these changes Jul 22, 2022

View reviewed changes

davidiw added 3 commits July 22, 2022 09:14

transaction-builder-generator -> aptos-sdk-builder

faa5b64

[aptos-sdk-builder] reintroduce and update generated payloads

3f9ce77

Now that the former transaction-generator-builder is healthy, I've reintroduced it. I've removed the unnecessary encode_ prefix and updated the functions appropriately.

[types] remove VecBytes

554cf09

For move arguments, VecBytes and Vec<Vec<u8>> should be interchangeable. The former is more natural, since theoretically, we can have nested vectors of other types.

davidiw force-pushed the move branch from aa33ece to daae114 Compare July 22, 2022 16:15

davidiw enabled auto-merge (rebase) July 22, 2022 16:17

davidiw force-pushed the move branch from daae114 to fdf9e97 Compare July 22, 2022 16:21

areshand reviewed Jul 22, 2022

View reviewed changes

davidiw disabled auto-merge July 22, 2022 21:56

davidiw force-pushed the move branch from fdf9e97 to 3eca6dd Compare July 22, 2022 21:59

davidiw enabled auto-merge (rebase) July 22, 2022 21:59

davidiw force-pushed the move branch from 3eca6dd to 36682d8 Compare July 22, 2022 22:09

davidiw added 2 commits July 22, 2022 16:46

[move-e2e-tests] cleanup println

1aa93d4

davidiw force-pushed the move branch from 36682d8 to 1aa93d4 Compare July 22, 2022 23:46

davidiw merged commit 914654e into main Jul 23, 2022

davidiw deleted the move branch July 23, 2022 00:20

ibalajiarun mentioned this pull request Aug 23, 2022

[telemetry-service] Log Ingest Endpoint #3254

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[aptos-sdk-builder] Rebrand and re-emerge transaction-builder-generator #2151

[aptos-sdk-builder] Rebrand and re-emerge transaction-builder-generator #2151

davidiw commented Jul 22, 2022 •

edited by geekflyer

Loading

wrwg commented Jul 22, 2022

davidiw commented Jul 22, 2022 •

edited

Loading

wrwg commented Jul 22, 2022

davidiw commented Jul 22, 2022 •

edited

Loading

wrwg commented Jul 22, 2022

wrwg commented Jul 22, 2022

zekun000 Jul 22, 2022

davidiw Jul 22, 2022

zekun000 Jul 22, 2022

davidiw Jul 22, 2022

movekevin Jul 22, 2022

davidiw Jul 22, 2022

movekevin Jul 22, 2022

movekevin Jul 22, 2022

davidiw commented Jul 22, 2022

areshand Jul 22, 2022

davidiw Jul 22, 2022

areshand Jul 22, 2022 •

edited

Loading

davidiw Jul 22, 2022

github-actions bot commented Jul 22, 2022

github-actions bot commented Jul 22, 2022

github-actions bot commented Jul 23, 2022

		@@ -25,7 +25,7 @@ fn initial_aptos_version() {
		println!("@@@@@@@@@@@@ account {:?}", account);

[aptos-sdk-builder] Rebrand and re-emerge transaction-builder-generator #2151

[aptos-sdk-builder] Rebrand and re-emerge transaction-builder-generator #2151

Conversation

davidiw commented Jul 22, 2022 • edited by geekflyer Loading

wrwg commented Jul 22, 2022

davidiw commented Jul 22, 2022 • edited Loading

wrwg commented Jul 22, 2022

davidiw commented Jul 22, 2022 • edited Loading

wrwg commented Jul 22, 2022

wrwg commented Jul 22, 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

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

davidiw commented Jul 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

areshand Jul 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jul 22, 2022

✅ Forge test success

github-actions bot commented Jul 22, 2022

✅ Forge test success

github-actions bot commented Jul 23, 2022

✅ Forge test success

davidiw commented Jul 22, 2022 •

edited by geekflyer

Loading

davidiw commented Jul 22, 2022 •

edited

Loading

davidiw commented Jul 22, 2022 •

edited

Loading

areshand Jul 22, 2022 •

edited

Loading