Transaction Builder #41

stefan-mysten · 2024-10-22T21:13:49Z

This is the first iteration on the transaction builder. I expect that it is not 100% correct as there are some areas that I am not sure about, thus I decided to push a PR to get some feedback.

Might be easier to go over it in a call, so we can also do that.

crates/sui-transaction-builder/src/lib.rs

crates/sui-transaction-builder/src/object.rs

stefan-mysten · 2024-10-24T15:42:27Z

Thanks so much for the review @amnn 🚀

crates/sui-sdk-types/src/types/address.rs

crates/sui-transaction-builder/src/lib.rs

crates/sui-sdk-types/src/types/transaction/mod.rs

crates/sui-transaction-builder/src/error.rs

crates/sui-transaction-builder/src/lib.rs

stefan-mysten · 2024-10-30T15:55:15Z

Bring in the macros for identifier, function, objectId, typetag.

crates/sui-transaction-builder/src/lib.rs

…module and expose the whole module

…le, and enhance the `set_gas` function to accept an Input as well

stefan-mysten · 2024-11-11T21:55:35Z

@bmwill @amnn thanks for all the feedback. I should have addressed all of it. The one thing that remains to do is to bring in/define some of the macros obj!(), input!() to make it easier to create the needed data. I will do this in a subsequent PR so that we can finally land this.

bmwill · 2024-11-12T14:53:48Z

why do we need macros? I tend to have an aversion to user-facing macros as they make things more complex rather than simpler from the perspective of an end user

amnn

Couple of themes in this round of comments:

Taking a closer look at the error story (I may be missing some details but the error module has now gotten quite a bit more complicated and I didn't spot a great indication as to why), e.g.
- Why are we depending on thiserror but then not using it.
- ConversionError is not very structured.
- What's the motivation for the overall error type, boxing the BuilderError?
Consistent use of parse() instead of from_str where possible.
Use TransactionEffectsAPI to avoid having to check for the V2 effects structure all the time.

And then some larger individual comments:

The upgrade example doesn't seem like it should work and needs another look.
I wasn't very clear about my suggestion to use Base64 to encode pure arguments -- that only applies to when we are sending it over the wire. We shouldn't be stashing bytes in a String at rest.
Avoiding having the raw module bytes stored inline on the stack for tests -- is there a way we could compile these as part of the test process?

Then there are some smaller one-off suggestions and comments as well.

crates/sui-sdk-types/src/types/transaction/unresolved.rs

crates/sui-transaction-builder/src/lib.rs

amnn · 2024-11-12T14:57:37Z

crates/sui-transaction-builder/src/lib.rs

+        let effects = client.execute_tx(vec![sig], &tx).await;
+        let mut package_id: Option<ObjectId> = None;
+        let mut created_objs = vec![];
+        if let Ok(Some(ref effects)) = effects {


use TransactionEffectsAPI.

We don't have one yet :D

we definitely do not want to introduce an TransactionEffectsApi trait, instead methods should just be added directly to those types

Will do, thanks.

It's a bit unclear to me what's the right way. I was thinking that this should be fine for a few fields: status, epoch, gas_used, transaction_digest, events_digest. For example, I added this:

impl TransactionEffects { pub fn status(&self) -> ExecutionStatus { match self { V1(e) => e.status, V2(e) => e.status } } }

Is this what you were suggesting @bmwill or more along the lines that both V1 and V2 should have their separate accessor implementations?

Ah, at the time that I made this comment, I was under the impression that this was a sui-types TransactionEffects, so yes, we don't need to add the trait here, or use enum_dispatch (IIUC, that is the main push back from Brandon) but for the sake of SDK users, we will need to offer the interface that enum dispatch would have generated, just manually (so each variant needs to have accessors that offer a common interface, and then the enum with versions needs to offer the same API dispatched on the enum version).

Yes the top level wrapper should provide an api that is easier to use and it should directly call methods as implemented on each inner version type.

amnn · 2024-11-12T14:58:02Z

crates/sui-transaction-builder/src/lib.rs

+        let (address, pk, _) = helper_setup(&mut tx, &client).await;
+
+        // publish very dummy package (from sui/examples/move/first_package)
+        let modules: [u8; 506] = [


Where did we get to on compiling these as part of the test, rather than hard coding module bytes?

I'm not against having raw compiled bytes here, if we do want to compile the bytes as a part of the test then we'd need to build utilities to shell out to the move compiler or take a dep on the mono-repo for that (which would be nice to avoid)

To avoid duplicating code, we could use the sui binary to do a sui move build --dump-bytecode-base64 and get the compiled modules, digest, and dependencies from there.
The advantage is that we're not copy pasting code and we're already using the CLI to start a local network for tests, so this seems reasonable to me.

Let me know 👍 or 👎

I'm happy with the shelling out approach, IIUC you are already using a Makefile as a test runner, so you could use that to generate test fixtures into a directory before the test starts -- seems fine to me.

Thinking on this more, i actually don't know what we're trying to test by calling the move compiler as a part of testing here. what we have now is imo easier, with the caveat that we should include instructions on how to update this if needed in the future

stefan-mysten · 2024-11-12T19:22:18Z

why do we need macros? I tend to have an aversion to user-facing macros as they make things more complex rather than simpler from the perspective of an end user

Would love to hear more about other worries are there with these macros. One upside to having these kind of macros is that they will be checks at compile time rather than runtime (e.g., parsing addresses, object ids, etc). It seems that the community likes this? (see MystenLabs/sui#20214)

bmwill · 2024-11-12T19:42:45Z

all such checks can be done at compiletime using normal const functions and would be the preferable approach

stefan-mysten · 2024-11-13T04:47:11Z

all such checks can be done at compiletime using normal const functions and would be the preferable approach

Right, cool. Would that mean that we need to reimplement Address::from_hex as as const function?

…e it nicer to pass in gas objects / objects as inputs

stefan-mysten · 2024-11-13T06:12:25Z

crates/sui-sdk-types/src/types/effects/v1.rs

@@ -15,7 +15,7 @@ use crate::types::TransactionEventsDigest;
 pub struct TransactionEffectsV1 {
    /// The status of the execution
    #[cfg_attr(feature = "schemars", schemars(flatten))]
-    status: ExecutionStatus,
+    pub status: ExecutionStatus,


All fields in V2 are pub while none are pub in V1. We should probably open them up/provide accessors?

stefan-mysten · 2024-11-13T06:17:49Z

Couple of themes in this round of comments:

* Taking a closer look at the error story (I may be missing some details but the `error` module has now gotten quite a bit more complicated and I didn't spot a great indication as to why), e.g.

The error story changed a bit to allow more flexible errors on the user side. We're experimenting a bit on what's the right way to propagate errors, and how should the error story look like.

amnn

Pulling out high level responses as the discussion has gotten quite long now:

All good to not take the mono-repo as a cargo dependency, but to use the sui CLI to build test fixtures, as part of the test process (as a separate step in the Makefile).
All good to not use enum_dispatch but to manually write out the same interface, without the trait. This work probably belongs in a separate PR.
All good to go for const fn instead of macros -- the main aim is to offer a way for people to specify identifiers and addresses, types, modules and functions without having to parse them at runtime, which requires lots of unwrapping. It's not clear to me how to make the kind of parsing involved their const-safe, which is why I would probably opt for a procedural macro that accepts a string literal, if given the choice, but if you know how to do that @bmwill, then const fn is the preferable choice.

amnn · 2024-11-13T12:38:09Z

crates/sui-transaction-builder/src/error.rs

+    }
+}
+
+impl std::fmt::Display for Error {


I'm not sure I understand the style comment, but thiserror supports specifying a source, and automatic conversion from another error, using #[from] which implies #[source]:

#[derive(thiserror::Error) pub enum Error { #[error("Transaction builder error. Missing data: {0}")] MissingData(#[from] MissingDataError), #[error("Transaction builder error. Conversion error: {0}")] ConversionError(#[from] ConversionError), }

Is there a technical reason not to use these features to write 20 times less code?

amnn · 2024-11-13T12:44:57Z

crates/sui-transaction-builder/src/lib.rs

+        Self {
+            kind: Some(unresolved::InputKind::Pure),
+            value: Some(unresolved::Value::String(base64ct::Base64::encode_string(
+                &bcs::to_bytes(value.0).unwrap(),
+            ))),
+            object_id: None,
+            version: None,
+            digest: None,
+            mutable: None,
+        }


My suggestion is doing the same thing as the code it replaced, just shorter and avoiding the duplication.

amnn · 2024-11-13T12:48:26Z

crates/sui-transaction-builder/src/lib.rs

+        let effects = client.execute_tx(vec![sig], &tx).await;
+        let mut package_id: Option<ObjectId> = None;
+        let mut created_objs = vec![];
+        if let Ok(Some(ref effects)) = effects {


Ah, at the time that I made this comment, I was under the impression that this was a sui-types TransactionEffects, so yes, we don't need to add the trait here, or use enum_dispatch (IIUC, that is the main push back from Brandon) but for the sake of SDK users, we will need to offer the interface that enum dispatch would have generated, just manually (so each variant needs to have accessors that offer a common interface, and then the enum with versions needs to offer the same API dispatched on the enum version).

bmwill · 2024-11-13T20:37:14Z

While const functions would improve things, i think that it should be out of scope for this particular pr

amnn · 2024-11-14T15:04:47Z

Agreed, I think the only open question for me in this PR are some of the decisions around the error story. Accessors for TransactionEffects and the helpers for generating IDs/addresses/types/functions should wait for follow-ups.

stefan-mysten requested a review from bmwill as a code owner October 22, 2024 21:13

stefan-mysten requested a review from amnn October 22, 2024 21:13

stefan-mysten commented Oct 22, 2024

View reviewed changes

crates/sui-transaction-builder/src/lib.rs Outdated Show resolved Hide resolved

stefan-mysten commented Oct 22, 2024

View reviewed changes

crates/sui-transaction-builder/src/lib.rs Outdated Show resolved Hide resolved

stefan-mysten commented Oct 22, 2024

View reviewed changes

crates/sui-transaction-builder/src/lib.rs Outdated Show resolved Hide resolved

stefan-mysten commented Oct 22, 2024

View reviewed changes

crates/sui-transaction-builder/src/lib.rs Outdated Show resolved Hide resolved

stefan-mysten commented Oct 22, 2024

View reviewed changes

crates/sui-transaction-builder/src/lib.rs Show resolved Hide resolved

amnn reviewed Oct 24, 2024

View reviewed changes

stefan-mysten force-pushed the tx_builder branch 3 times, most recently from 28d7deb to 17d7187 Compare October 24, 2024 23:27

stefan-mysten requested a review from amnn October 25, 2024 00:59

stefan-mysten force-pushed the tx_builder branch from 6e59091 to 693ffbc Compare October 29, 2024 16:52

stefan-mysten commented Oct 29, 2024

View reviewed changes

crates/sui-sdk-types/src/types/address.rs Show resolved Hide resolved

stefan-mysten commented Oct 29, 2024

View reviewed changes

crates/sui-transaction-builder/src/lib.rs Outdated Show resolved Hide resolved