Core Script Builder Codegen Logic #14168

runtian-zhou · 2024-07-30T21:02:38Z

Description

Implemented a library to

compile a list of intent calls to Script
decompile from a Script into a list of intent calls.

Type of Change

Which Components or Systems Does This Change Impact?

How Has This Been Tested?

Published round trip tests to make sure they can be compiled and decompiled correctly.

Key Areas to Review

For the decompilation logic, we will be hosting this logic as part of the api service so it's important to make sure it will not crash.

Checklist

I have read and followed the CONTRIBUTING doc
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I identified and added all stakeholders and component owners affected by this change as reviewers
I tested both happy and unhappy path of the functionality
I have made corresponding changes to the documentation

trunk-io · 2024-07-30T21:02:42Z

⏱️ 4m total CI duration on this PR

Job	Cumulative Duration	Recent Runs
check-dynamic-deps	2m	🟩
dispatch_event	1m	🟥

🚨 1 job on the last run was significantly faster/slower than expected

Job	Duration	vs 7d avg	Delta
check-dynamic-deps	2m	1m

_{settings ⋅ feedback ⋅ docs ⋅ learn more about trunk.io}

runtian-zhou · 2024-07-30T21:08:40Z

aptos-move/aptos-intent/src/codegen.rs

+}
+
+#[derive(Serialize, Deserialize)]
+pub(crate) struct Script {


This is a hack as aptos_types cannot be compiled into WASM, so I have to avoid using it as a dependency.

codecov · 2024-07-30T21:15:23Z

Codecov Report

Attention: Patch coverage is 0% with 273 lines in your changes missing coverage. Please review.

Project coverage is 60.0%. Comparing base (18bcbd2) to head (7acdd25).
Report is 42 commits behind head on main.

Files with missing lines	Patch %	Lines
...hird_party/move/move-binary-format/src/builders.rs	0.0%	256 Missing ⚠️
third_party/move/move-core/types/src/parser.rs	0.0%	14 Missing ⚠️
...party/move/move-core/types/src/language_storage.rs	0.0%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##             main   #14168     +/-   ##
=========================================
+ Coverage    59.8%    60.0%   +0.2%     
=========================================
  Files         853      857      +4     
  Lines      207949   210898   +2949     
=========================================
+ Hits       124354   126555   +2201     
- Misses      83595    84343    +748

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

vgao1996

Some high level comments

vgao1996 · 2024-07-30T23:06:11Z

aptos-move/aptos-intent/src/codegen.rs

@@ -0,0 +1,489 @@
+// Copyright © Aptos Foundation


Wondering if we should move this into move-binary-format. A builder library would have a lot of use cases besides the scope of the intent engine

I think parts of the code can potentially go to move-binary-format? But this would require some refactoring to be able to handle both module and script so I would avoid doing that in this PR.

vgao1996 · 2024-07-30T23:07:52Z

aptos-move/aptos-intent/src/decompiler.rs

@@ -0,0 +1,235 @@
+// Copyright © Aptos Foundation


Love this approach. One question though: when do we decide to run this decompiler? When we see some special flag in the metadata? I'm just thinking more about the unhappy path where you have something that isn't really an intent script.

Also playing the devil's advocate: how do you envision the long term maintainability of this mini decompiler if we continue to add more functionalities?

This is a very good point. Since the decompiler will be served as part of the full node we need extra eyes on this logic to make sure it won't crash anything.

For longer term maintainability I think we need to maintain the tests to make sure the round trip equality holds?

vgao1996

1/n Took a look at codegen.rs. Guess my main hope is for this to become a more general bytecode manipulation library other clients can also depend on.

vgao1996 · 2024-07-30T23:11:48Z

aptos-move/aptos-intent/src/codegen.rs

+        if let Some(result) = self.address_pool.get(address) {
+            return Ok(*result);
+        }
+        if self.script.address_identifiers.len() >= TableIndex::MAX as usize {
+            return Err(PartialVMError::new(StatusCode::INDEX_OUT_OF_BOUNDS));
+        }
+        let return_idx = AddressIdentifierIndex(self.script.address_identifiers.len() as u16);
+        self.script.address_identifiers.push(*address);
+        self.address_pool.insert(*address, return_idx);
+        Ok(return_idx)


Nit: can use the entry() API to save a lookup.

It might also be worth coming up with some higher order helpers to perform the common "get or insert" op. For reference, here's some similar stuff I did in my gov proposal simulation PR

fn get_or_add<T: PartialEq>(pool: &mut Vec<T>, val: T) -> usize { match pool.iter().position(|elem| elem == &val) { Some(idx) => idx, None => { let idx = pool.len(); pool.push(val); idx }, } }

vgao1996 · 2024-07-30T23:17:32Z

aptos-move/aptos-intent/src/codegen.rs

+            return Ok(*result);
+        }
+        if self.script.module_handles.len() >= TableIndex::MAX as usize {
+            return Err(PartialVMError::new(StatusCode::INDEX_OUT_OF_BOUNDS));


I know we have a history of abusing vm errors in tools, but wonder if we would benefit from this not repeating the same pattern.

I think it would be fine this case since all errors will be turned into anyhow error here? Although I agree that we could remove those deps. WDYT?

vgao1996 · 2024-07-30T23:20:47Z

aptos-move/aptos-intent/src/codegen.rs

+use serde::{Deserialize, Serialize};
+use std::collections::BTreeMap;
+
+#[derive(Default)]


If we want to make this a general-use library, then I feel like we'll need to come up with a way to make it work with an existing non-empty CompiledScript/Module.

Ok now I see your point. Basically you want to get rid of the intent codegen logic and move the logic to the builder, so that what's remaining would be purely reusable code for manipulating compiled script.

vgao1996 · 2024-07-30T23:22:41Z

aptos-move/aptos-intent/src/codegen.rs

+        if let Some(result) = self.address_pool.get(address) {
+            return Ok(*result);
+        }
+        if self.script.address_identifiers.len() >= TableIndex::MAX as usize {
+            return Err(PartialVMError::new(StatusCode::INDEX_OUT_OF_BOUNDS));
+        }


Yeah, the logic here assumes that if something is not in the hashmap then it also does not exist in the script. Fine if we're starting from an empty script, but personally I'd like to see this becoming a general library we can use to modify CompiledModule and CompiledScript

vgao1996 · 2024-07-30T23:25:20Z

aptos-move/aptos-intent/src/codegen.rs

+        Err(PartialVMError::new(StatusCode::LOOKUP_FAILED))
+    }
+
+    fn compile_batched_call(


Wonder if this belongs to builder.rs

vgao1996 · 2024-07-30T23:27:40Z

aptos-move/aptos-intent/src/codegen.rs

+}
+
+#[derive(Serialize, Deserialize)]
+pub(crate) struct Script {


Same, feel like it might be helpful to separate out for CompiledScript building and (Aptos) transaction script generation. The added modularity will benefit other use cases that requires bytecode manipulation.

vgao1996 · 2024-07-30T23:29:05Z

aptos-move/aptos-intent/src/codegen.rs

+    function_pool: BTreeMap<(ModuleHandleIndex, IdentifierIndex), FunctionHandleIndex>,
+    struct_pool: BTreeMap<(ModuleHandleIndex, IdentifierIndex), StructHandleIndex>,
+    signature_pool: BTreeMap<Signature, SignatureIndex>,
+    script: CompiledScript,


If timing permits, I'd love to see this evolving into a builder that supports both CompiledScript and CompiledModule. This doesn't mean you need to add all the import functions though -- those can be added on-demand.

brmataptos

A few superficial comments. Still digesting...

brmataptos · 2024-07-31T01:02:25Z

third_party/move/move-core/types/src/parser.rs

+                    Token::Name(s) => ModuleId::new(AccountAddress::from_hex_literal(&addr)?, Identifier::new(s)?),
+                    tok => bail!("unexpected token {:?}, expected string", tok),
+                }
+                tok =>  bail!("unexpected token {:?}, expected string", tok),


Should you mention that :: is also allowed here in your error?

brmataptos · 2024-07-31T01:03:30Z

third_party/move/move-core/types/src/parser.rs

+                Token::Name(s) => ModuleId::new(AccountAddress::from_hex_literal(&addr)?, Identifier::new(s)?),
+                Token::ColonColon => match self.next()? {
+                    Token::Name(s) => ModuleId::new(AccountAddress::from_hex_literal(&addr)?, Identifier::new(s)?),
+                    tok => bail!("unexpected token {:?}, expected string", tok),


string --> identifier

below, also.

brmataptos · 2024-07-31T01:04:52Z

third_party/move/move-core/types/src/parser.rs

+    fn parse_module_id(&mut self) -> Result<ModuleId> {
+        Ok(match self.next()? {
+            Token::Address(addr) => match self.next()? {
+                Token::Name(s) => ModuleId::new(AccountAddress::from_hex_literal(&addr)?, Identifier::new(s)?),


So you allow Token::Address followed by Token::Name with nothing in between? That seems odd.

aptos-move/aptos-intent/src/builder.rs

brmataptos · 2024-07-31T01:18:14Z

aptos-move/aptos-intent/src/builder.rs

+        let (module, handle) = find_function(&self.modules, module_id, func_name)?;
+        let mut returns = vec![];
+        for (idx, sig) in module.signature_at(handle.return_).0.iter().enumerate() {
+            let call_idx = (self.calls.len() - 1) as u16;


if self.calls.len() is 0 this underflows and behavior differs between debug and release mode. Not a great idea.

return_values will always be called after self.calls is pushed so we should be fine. With that said tho it's always good to add an extra check here.

brmataptos · 2024-07-31T01:25:53Z

aptos-move/aptos-intent/Cargo.toml

+
+[dev-dependencies]
+e2e-move-tests = { path = "../e2e-move-tests" }
+aptos-types = { workspace = true }


I assume the funky symbol means there's no trailing \n on this file? I think you need one to be POSIX-compliant.

vgao1996 · 2024-08-01T20:42:02Z

third_party/move/move-binary-format/src/builders.rs

+    vm_status::StatusCode,
+};
+
+fn get_or_add<T: PartialEq>(pool: &mut Vec<T>, val: T) -> PartialVMResult<usize> {


A bit sad to see the caching gone here. Without caching it could be O(n^2) complexity to build a complex script. If another component on the production path picks this up it could also run into time complexity issues unexpectedly, albeit not at the same severity as the resource viewer.

github-actions · 2024-09-16T02:02:40Z

This issue is stale because it has been open 45 days with no activity. Remove the stale label, comment or push a commit - otherwise this will be closed in 15 days.

wrwg · 2024-10-10T02:29:07Z

Cargo.toml

@@ -31,6 +31,7 @@ members = [
    "aptos-move/aptos-vm-types",
    "aptos-move/aptos-workspace-server",
    "aptos-move/block-executor",
+    "aptos-move/dynamic-transaction-composer",


Not that important, but I think on this level transaction-composer would be good enough, 'dynamic' is more like an extra marketing attribute. Moving forward, we will just call it on technical level the 'transaction composer' anyway, three words too long.

I'll rename this folder after a stamp. Brian suggested that renaming will dismiss previous commments

wrwg · 2024-10-10T02:34:37Z

aptos-move/dynamic-transaction-composer/src/builder.rs

@@ -0,0 +1,498 @@
+// Copyright © Aptos Foundation
+// SPDX-License-Identifier: Apache-2.0
+


A short module description? We try to do this in the compiler now and it is really helpful. (//! doc)

Added description.

aptos-move/dynamic-transaction-composer/src/builder.rs

wrwg · 2024-10-10T02:49:45Z

aptos-move/dynamic-transaction-composer/src/builder.rs

+}
+
+#[wasm_bindgen(js_name = BatchArgument)]
+/// Arguments for each function. Wasm bindgen only support C-style enum so use option to work around.


I'm not quite understanding this restriction. At https://rustwasm.github.io/wasm-bindgen/?search=enum%20t I couldn't find anything either. Looks like bindgen supports the full JS value system, is then perhaps a matter to use serde to convert some of our Rust data types to JS before we pass it out? Please elaborate.

rustwasm/wasm-bindgen#2407

aptos-move/dynamic-transaction-composer/src/codegen.rs

wrwg · 2024-10-10T03:32:04Z

aptos-move/dynamic-transaction-composer/src/codegen.rs

+struct Context {
+    script: CompiledScript,
+    // Arguments to the compiled script.
+    args: Vec<Vec<u8>>,


I don't quite understand what this here is. The BCS of a set of concrete arguments? Why would this be on this level?

I would the compiled script expected to be parametric over a set of formal parameters, so it can be stored and executed many times with different parameters.

aptos-move/dynamic-transaction-composer/src/codegen.rs

brmataptos · 2024-10-10T00:56:36Z

aptos-move/dynamic-transaction-composer/Cargo.toml

+move-binary-format = { workspace = true }
+move-bytecode-verifier = { workspace = true }
+move-core-types = { workspace = true }
+reqwest = { version = "*", features = ["blocking"] }


Why not workspace here?

The reason why I'm avoiding workspace is this repo will be compiled into WASM and thus might have different dependencies and I don't want to pollute the main workspace because of that.

Yeah, I started to think how we deal with THAT. As a matter of fact, you have to pull in significant other parts of the code which already have workspace dependencies. We need a more general solution for this problem. I'm not sure whether there is something like 'dependency override' in cargo. Or perhaps for the WASM build, we need to generate the build config by our own script, that is generate source for a crate then compile it. For now I think this should be workspace too, it doesn't appear to solve the problem this way.

Still needs resolution. Not using workspace doesn't help here. Also "*" is not our practice, should be a concrete version.

vgao1996

1/3 The third-party/move changes all look good to me

vgao1996 · 2024-10-18T22:20:37Z

third_party/move/move-binary-format/src/builders.rs

+        self.add_signature(sig)
+    }
+
+    pub fn add_signature(&mut self, signature: Signature) -> PartialVMResult<SignatureIndex> {


@runtian-zhou can correct me if I'm wrong, but it seems like import_ is specifically for taking things from another module (see they all take compiled module + index as arguments), while here add_signature doesn't care about the origin of the signature added.

third_party/move/move-binary-format/src/builders.rs

third_party/move/move-binary-format/src/file_format.rs

vgao1996

2/3 Builder

The main logic looks good. Some questions on the implementation details

Also wondering if you plan to offer a textual representation for the input. It could be a simple register-based DSL

(a, b) = foo();
bar(a);
baz(&b, &b);

aptos-move/dynamic-transaction-composer/src/builder.rs

vgao1996 · 2024-10-19T00:44:44Z

aptos-move/dynamic-transaction-composer/src/builder.rs

+                Argument::Raw(bytes) => {
+                    let new_local_idx = self.parameters_ty.len() as u16;
+                    self.parameters_ty.push(ty);
+                    self.parameters.push(bytes);
+                    arguments.push(AllocatedLocal {
+                        op_type: ArgumentOperation::Move,
+                        is_parameter: true,
+                        local_idx: new_local_idx,
+                    })
+                },


Is it necessary to allocate a local for this argument?

Another related question: do we worry about running out of locals? There are only 256 of them IIRC

IMO there are two ways to do it:

Import the argument into constant pool

Assign a parameter location and pass it as argument to the script.

I would personally prefer 2 because it makes script caching a bit easier, as the script body part would remain the same if there's a list of commonly composed script.

On running out of locals, this is indeed a concern. There are only 256 of them and we need it for # of signers + # of parameters + # of returns. I don't want to over optimize for it rightaway but I don't see a clear path getting rid of the constraint rightway either.

I would personally prefer 2 because it makes script caching a bit easier, as the script body part would remain the same if there's a list of commonly composed script.

So all Argument::Raw are exposed as script parameters? If so, I find the idea both intriguing and worrying. It's good that we may be able to cache more stuff, but it also feels like a very leaky abstraction -- users may be surprised by the extra parameters the builder inserted.

There are only 256 of them and we need it for # of signers + # of parameters + # of returns.

I was more thinking about returns. Wondering if we could make more use of the stack. Agreed it's not necessarily something we need to optimize at this moment though.

I don't want to over optimize for it rightaway but I don't see a clear path getting rid of the constraint rightway either.

Relaxing it at the file format/vm level is probably not that hard (but still a nasty chore). @wrwg just curious, would the bytecode verifier have trouble with, let's say, 1024 locals?

vgao1996 · 2024-10-19T00:46:43Z

aptos-move/dynamic-transaction-composer/src/builder.rs

+                Argument::Local(AllocatedLocal {
+                    op_type: ArgumentOperation::Move,
+                    is_parameter: false,
+                    local_idx: idx,
+                })


Conceptually, I wonder if you should return to the caller a "location" without the operation type, and allow them to specify their intent later.

The nice thing here is that the api would be defaulted to Move behavior. I think this will make the TS sdk interface look very clean as they can pass the returned value directly as arugment to the next call, which actually aligns with the logic here.

I see your reasoning here. Although I wonder if this is better done at a higher level (e.g. the register DSL I mentioned). Anyway, not a blocker for the current implemetation

aptos-move/dynamic-transaction-composer/src/lib.rs

aptos-move/dynamic-transaction-composer/src/builder.rs

vgao1996 · 2024-10-19T00:52:49Z

aptos-move/dynamic-transaction-composer/src/builder.rs

+    }
+
+    /// Load up a module from a remote endpoint. Will need to invoke this function prior to the call.
+    pub async fn load_module(


I'm not against the fetching modules from remote, but I wonder if we should provide an option for supplying them (or ABIs) locally, potentially for saving the turnaround time + better service availability.

vgao1996

3/3 Decompiler looks good. Although I didn't have the mental capacity to check if the (local_idx -> prev_result) remapping is done 100% correctly.

aptos-move/dynamic-transaction-composer/src/decompiler.rs

wrwg

Please indicate clearer what comments are resolved, most of my previous comments have not been acked or resolved. I'd prefer to not figuring this out by reading everything again.

wrwg · 2024-10-20T22:05:38Z

aptos-move/dynamic-transaction-composer/Cargo.toml

+serde = { workspace = true }
+serde_bytes = { workspace = true }
+serde_json = { workspace = true }
+tsify-next = "0.5.4"


aptos-move/dynamic-transaction-composer/src/builder.rs

wrwg · 2024-10-20T22:09:43Z

aptos-move/dynamic-transaction-composer/src/builder.rs

+}
+
+#[wasm_bindgen]
+pub struct BatchedFunctionCallBuilder {


Please address

wrwg · 2024-10-20T22:11:19Z

aptos-move/dynamic-transaction-composer/src/builder.rs

+use wasm_bindgen::prelude::*;
+
+#[wasm_bindgen]
+#[derive(Tsify, Clone, Serialize, Deserialize, PartialEq, Eq, Debug)]


Doc comments on all public entities, please.

aptos-move/dynamic-transaction-composer/src/builder.rs

vgao1996

LGTM. Let's get the first version landed

vgao1996 · 2024-10-29T16:44:06Z

aptos-move/dynamic-transaction-composer/src/lib.rs

+#[derive(Clone, Debug, Hash, Eq, PartialEq, Serialize, Deserialize)]
+pub struct PreviousResult {
+    /// Refering to the return value in the `call_idx`th call.
+    call_idx: u16,
+    /// Refering to the `return_idx`th return value in that call, since move function call can
+    /// return multiple values.
+    return_idx: u16,
+    /// How this result would be used.
+    operation_type: ArgumentOperation,
+}
+
+/// Arguments to the `MoveFunctionCall`.
+#[derive(Clone, Debug, Hash, Eq, PartialEq, Serialize, Deserialize, Tsify)]
+#[tsify(into_wasm_abi, from_wasm_abi)]
+pub enum CallArgument {
+    /// Passing raw bytes to the function. The bytes must follows the existing constraints for
+    /// transaction arguments.
+    Raw(Vec<u8>),
+    /// Refering to signer of the transaction. If it's a single signer transaction you will only
+    /// be able to access `Signer(0)`. You will be able to access other signers if it's a multi
+    /// agent transaction.
+    Signer(u16),
+    /// The arugment came from the returned value of a previous `MoveFunctionCall`.
+    PreviousResult(PreviousResult),
+}
+
+/// How to consume the returned value coming from a previous `MoveFunctionCall`.
+#[derive(Clone, Debug, Hash, Eq, PartialEq, Serialize, Deserialize)]
+pub enum ArgumentOperation {
+    /// Move the returned value to the next caller. This can be used for values that don't have
+    /// `copy` ability.
+    Move,
+    /// Copy the returned value and pass it to the next caller.
+    Copy,
+    /// Borrow an immutable reference from a returned value and pass it to the next caller.
+    Borrow,
+    /// Borrow a mutable reference from a returned value and pass it to the next caller.
+    BorrowMut,
+}


Love the changes here.

github-actions · 2024-10-30T18:54:54Z

✅ Forge suite `realistic_env_max_load` success on `a09aef7be1bd5595f50608dd5d5f5c6620ece50d`

two traffics test: inner traffic : committed: 14234.88 txn/s, latency: 2798.45 ms, (p50: 2700 ms, p70: 2700, p90: 3000 ms, p99: 12300 ms), latency samples: 5412440
two traffics test : committed: 99.90 txn/s, latency: 2688.32 ms, (p50: 1400 ms, p70: 1500, p90: 9800 ms, p99: 13400 ms), latency samples: 1840
Latency breakdown for phase 0: ["MempoolToBlockCreation: max: 1.982, avg: 1.540", "ConsensusProposalToOrdered: max: 0.319, avg: 0.296", "ConsensusOrderedToCommit: max: 0.406, avg: 0.381", "ConsensusProposalToCommit: max: 0.705, avg: 0.677"]
Max non-epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 0.91s no progress at version 8542 (avg 0.21s) [limit 15].
Max epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 8.47s no progress at version 2883418 (avg 8.47s) [limit 15].
Test Ok

github-actions · 2024-10-30T18:57:26Z

✅ Forge suite `framework_upgrade` success on `9c922ebe94f5ff4b58df4617f3ff003e2ce10ccd` ==> `a09aef7be1bd5595f50608dd5d5f5c6620ece50d`

Compatibility test results for 9c922ebe94f5ff4b58df4617f3ff003e2ce10ccd ==> a09aef7be1bd5595f50608dd5d5f5c6620ece50d (PR)
Upgrade the nodes to version: a09aef7be1bd5595f50608dd5d5f5c6620ece50d
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1166.51 txn/s, submitted: 1168.30 txn/s, failed submission: 1.79 txn/s, expired: 1.79 txn/s, latency: 2749.08 ms, (p50: 2400 ms, p70: 3000, p90: 4800 ms, p99: 6600 ms), latency samples: 104220
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1250.37 txn/s, submitted: 1252.82 txn/s, failed submission: 2.45 txn/s, expired: 2.45 txn/s, latency: 2382.14 ms, (p50: 2100 ms, p70: 2400, p90: 4300 ms, p99: 5600 ms), latency samples: 112380
5. check swarm health
Compatibility test for 9c922ebe94f5ff4b58df4617f3ff003e2ce10ccd ==> a09aef7be1bd5595f50608dd5d5f5c6620ece50d passed
Upgrade the remaining nodes to version: a09aef7be1bd5595f50608dd5d5f5c6620ece50d
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1278.04 txn/s, submitted: 1280.71 txn/s, failed submission: 2.67 txn/s, expired: 2.67 txn/s, latency: 2528.24 ms, (p50: 2100 ms, p70: 2700, p90: 4500 ms, p99: 5900 ms), latency samples: 114820
Test Ok

github-actions · 2024-10-30T18:57:56Z

✅ Forge suite `compat` success on `9c922ebe94f5ff4b58df4617f3ff003e2ce10ccd` ==> `a09aef7be1bd5595f50608dd5d5f5c6620ece50d`

Compatibility test results for 9c922ebe94f5ff4b58df4617f3ff003e2ce10ccd ==> a09aef7be1bd5595f50608dd5d5f5c6620ece50d (PR)
1. Check liveness of validators at old version: 9c922ebe94f5ff4b58df4617f3ff003e2ce10ccd
compatibility::simple-validator-upgrade::liveness-check : committed: 14734.56 txn/s, latency: 2270.09 ms, (p50: 1900 ms, p70: 2100, p90: 3900 ms, p99: 6900 ms), latency samples: 487460
2. Upgrading first Validator to new version: a09aef7be1bd5595f50608dd5d5f5c6620ece50d
compatibility::simple-validator-upgrade::single-validator-upgrading : committed: 6825.65 txn/s, latency: 4167.32 ms, (p50: 4500 ms, p70: 5000, p90: 5200 ms, p99: 5200 ms), latency samples: 128340
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 6901.74 txn/s, latency: 4689.52 ms, (p50: 5100 ms, p70: 5200, p90: 5800 ms, p99: 6300 ms), latency samples: 236680
3. Upgrading rest of first batch to new version: a09aef7be1bd5595f50608dd5d5f5c6620ece50d
compatibility::simple-validator-upgrade::half-validator-upgrading : committed: 6557.76 txn/s, latency: 4360.41 ms, (p50: 5000 ms, p70: 5300, p90: 5400 ms, p99: 5500 ms), latency samples: 122580
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 6269.70 txn/s, latency: 5126.57 ms, (p50: 5300 ms, p70: 5300, p90: 6900 ms, p99: 7200 ms), latency samples: 210180
4. upgrading second batch to new version: a09aef7be1bd5595f50608dd5d5f5c6620ece50d
compatibility::simple-validator-upgrade::rest-validator-upgrading : committed: 8339.81 txn/s, latency: 3314.83 ms, (p50: 3600 ms, p70: 3900, p90: 4100 ms, p99: 5100 ms), latency samples: 148120
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 8373.14 txn/s, latency: 3772.14 ms, (p50: 3700 ms, p70: 4000, p90: 5900 ms, p99: 6500 ms), latency samples: 275660
5. check swarm health
Compatibility test for 9c922ebe94f5ff4b58df4617f3ff003e2ce10ccd ==> a09aef7be1bd5595f50608dd5d5f5c6620ece50d passed
Test Ok

runtian-zhou requested review from vineethk, vgao1996, wrwg, fEst1ck and brmataptos July 30, 2024 21:02

runtian-zhou commented Jul 30, 2024

View reviewed changes

vgao1996 reviewed Jul 30, 2024

View reviewed changes

brmataptos reviewed Jul 31, 2024

View reviewed changes

brmataptos self-requested a review July 31, 2024 01:28

runtian-zhou force-pushed the runtianz/aptos_intent_codegen_core branch 2 times, most recently from 0968d9b to 6d820a3 Compare August 1, 2024 08:30

vgao1996 reviewed Aug 1, 2024

View reviewed changes

github-actions bot added the Stale label Sep 16, 2024

github-actions bot closed this Oct 1, 2024

runtian-zhou reopened this Oct 2, 2024

runtian-zhou added 5 commits October 2, 2024 17:04

Core Intent Codegen Logic

4c198ba

Fix comments

087e681

fixup! Fix comments

1d94474

fixup! fixup! Fix comments

d1af682

fixup! fixup! fixup! Fix comments

cd28535

runtian-zhou force-pushed the runtianz/aptos_intent_codegen_core branch from 01b0823 to 12d67fd Compare October 3, 2024 00:06

github-actions bot removed the Stale label Oct 3, 2024

runtian-zhou force-pushed the runtianz/aptos_intent_codegen_core branch 2 times, most recently from ba4e83c to 03d36cd Compare October 3, 2024 20:41

fixup! fixup! fixup! fixup! Fix comments

f28aaed

runtian-zhou force-pushed the runtianz/aptos_intent_codegen_core branch from 03d36cd to f28aaed Compare October 3, 2024 23:24

wrwg reviewed Oct 10, 2024

View reviewed changes

brmataptos reviewed Oct 10, 2024

View reviewed changes

runtian-zhou force-pushed the runtianz/aptos_intent_codegen_core branch 2 times, most recently from ba6f516 to 1cec6a9 Compare October 17, 2024 23:08

Refactor

e78b872

runtian-zhou force-pushed the runtianz/aptos_intent_codegen_core branch from 1cec6a9 to e78b872 Compare October 17, 2024 23:10

vgao1996 reviewed Oct 18, 2024

View reviewed changes

vgao1996 reviewed Oct 19, 2024

View reviewed changes

aptos-move/dynamic-transaction-composer/src/decompiler.rs Outdated Show resolved Hide resolved

runtian-zhou added 2 commits October 19, 2024 00:06

unify arguments

6c5b96c

fix victor's comments

410bed9

wrwg reviewed Oct 20, 2024

View reviewed changes

runtian-zhou force-pushed the runtianz/aptos_intent_codegen_core branch 3 times, most recently from 6ad13e6 to b1d0201 Compare October 21, 2024 20:56

vgao1996 approved these changes Oct 29, 2024

View reviewed changes

wrwg approved these changes Oct 30, 2024

View reviewed changes

runtian-zhou added 2 commits October 30, 2024 11:01

fix WG's comments

72b9ddf

fixup! fix WG's comments

43be219

runtian-zhou force-pushed the runtianz/aptos_intent_codegen_core branch from 061598e to 43be219 Compare October 30, 2024 18:03

Merge branch 'main' into runtianz/aptos_intent_codegen_core

a09aef7

runtian-zhou enabled auto-merge (squash) October 30, 2024 18:03

This comment has been minimized.

Sign in to view

runtian-zhou merged commit 3bd001b into main Oct 30, 2024
82 of 94 checks passed

runtian-zhou deleted the runtianz/aptos_intent_codegen_core branch October 30, 2024 18:58

		@@ -0,0 +1,498 @@
		// Copyright © Aptos Foundation
		// SPDX-License-Identifier: Apache-2.0

Core Script Builder Codegen Logic #14168

Core Script Builder Codegen Logic #14168

Conversation

runtian-zhou commented Jul 30, 2024 • edited Loading

Description

Type of Change

Which Components or Systems Does This Change Impact?

How Has This Been Tested?

Key Areas to Review

Checklist

trunk-io bot commented Jul 30, 2024 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Jul 30, 2024 • edited Loading

Codecov Report

vgao1996 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

vgao1996 left a comment

Choose a reason for hiding this comment

vgao1996 Jul 30, 2024 • 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

brmataptos 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

github-actions bot commented Sep 16, 2024

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

vgao1996 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vgao1996 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

vgao1996 Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

vgao1996 Oct 19, 2024 • edited Loading

Choose a reason for hiding this comment

vgao1996 left a comment

Choose a reason for hiding this comment

wrwg 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

vgao1996 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

runtian-zhou commented Jul 30, 2024 •

edited

Loading

trunk-io bot commented Jul 30, 2024 •

edited

Loading

codecov bot commented Jul 30, 2024 •

edited

Loading

vgao1996 Jul 30, 2024 •

edited

Loading

vgao1996 left a comment •

edited

Loading

vgao1996 Oct 21, 2024 •

edited

Loading

vgao1996 Oct 19, 2024 •

edited

Loading

✅ Forge suite `realistic_env_max_load` success on `a09aef7be1bd5595f50608dd5d5f5c6620ece50d`

✅ Forge suite `framework_upgrade` success on `9c922ebe94f5ff4b58df4617f3ff003e2ce10ccd` ==> `a09aef7be1bd5595f50608dd5d5f5c6620ece50d`

✅ Forge suite `compat` success on `9c922ebe94f5ff4b58df4617f3ff003e2ce10ccd` ==> `a09aef7be1bd5595f50608dd5d5f5c6620ece50d`