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

[Transaction Builder] Adding distinct interfaces for entry function and script transaction payloads #8

Merged
merged 11 commits into from
Oct 11, 2023

Conversation

xbtmatt
Copy link
Contributor

@xbtmatt xbtmatt commented Oct 11, 2023

Description

This is ideally the last significant change to anything BCS-related before alpha release. It looks like a lot but it's mostly just consolidating types, removing extraneous classes, and adding interfaces to avoid circular dependencies and make things more clear

1. Removed the ScriptTransactionArgument class

  • Developers do not need to know or care about it. We can handle it for them
  • There is now just a deserializeFromScriptArgument function we use in the Script class when deserializing the argument vector
  • This means the inputs to a Script function and an Entry function are now the same, with the exception of scripts not supporting Options and only MoveVector<U8>
  • Fixed lots of potential circular dependency issues

2. Added EntryFunctionBytes class

  • This is basically FixedBytes, except without a serializeForScriptFunction function.
  • This means you can't inadvertently use EntryFunctionBytes as ScriptFunctionArguments
  • This is the only reason the class exists. We can probably remove it, but now for it does provide some clarity

3. Added serializeForEntryFunction and serializeForScriptFunction for each class

  • Used in the TransactionArgument interface, which allows us to pass around Move values and serialize them according to the inferred transaction payload without having to have a class to represent those values
  • The intent behind each one is to make it very clear which classes are (or aren't) idempotent when serializing => deserializing => serializing for an entry function and for a script function

4. Added support for Objects and Strings in script functions

5. Renamed all instances and references of String and Option to have Move prepended to them

6. Added EntryFunctionArgumentTypes and ScriptFunctionArgumentTypes

  • This makes the inputs to transaction builder functiosn more stringent/clear
  • Need to test this and use it in general to see if the flow makes sense and/or the type unions work as expected

Still need to test if script payload args have compile time and/or runtime errors with incorrect argument types, and make more extensive tests around script payloads, especially using strings and objects.

Example of new ScriptPayload:

Now:

const payload = generateTransactionPayload({
  bytecode:
    "a11ceb0b ... 0302",   // see transaction_builder.test.ts
  type_arguments: [],
  arguments: [
    new U64(100),
    new U64(200),
    Account.generate({ scheme: SigningScheme.Ed25519 }).accountAddress,
    Account.generate({ scheme: SigningScheme.Ed25519 }).accountAddress,
    new U64(50),
  ],
});

Before:

const payload = generateTransactionPayload({
  bytecode: "a11ceb0b ... 0302",
  type_arguments: [],
  arguments: [
    new ScriptTransactionArgumentU64(BigInt(100)),
    new ScriptTransactionArgumentU64(BigInt(200)),
    new ScriptTransactionArgumentAddress(Account.generate({ scheme: SigningScheme.Ed25519 }).accountAddress),
    new ScriptTransactionArgumentAddress(Account.generate({ scheme: SigningScheme.Ed25519 }).accountAddress),
    new ScriptTransactionArgumentU64(BigInt(50)),
  ],
});

Test Plan

pnpm jest

Added basic test cases for things

@xbtmatt xbtmatt requested a review from 0xmaayan October 11, 2023 09:09
@xbtmatt xbtmatt marked this pull request as ready for review October 11, 2023 09:09
@xbtmatt xbtmatt requested a review from a team as a code owner October 11, 2023 09:09
Copy link
Collaborator

@0xmaayan 0xmaayan left a comment

Choose a reason for hiding this comment

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

left some questions and comments but overall lgtm!

src/bcs/index.ts Outdated Show resolved Hide resolved
src/api/transaction_submission.ts Outdated Show resolved Hide resolved
static U8(values: Array<number> | HexInput): MoveVector<U8> {
let numbers: Array<number>;

if (Array.isArray(values) && typeof values[0] === "number") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

we assume all elements in the array are numbers and not just the first one? or we actually check it somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

each one is eventually validated when you map the numbers I believe, but will check it here if not

Copy link
Contributor Author

@xbtmatt xbtmatt Oct 11, 2023

Choose a reason for hiding this comment

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

it's validated when mapping the numbers, since new U8(v) should validate it

Ah yes, I already have a test case for it in bcs-helper.test.ts:

      expect(() => MoveVector.U8(["01", "02", "03"] as any)).toThrow();

/**
* Deserialize a Script Transaction Argument
*/
export function deserializeFromScriptArgument(deserializer: Deserializer): TransactionArgument {
Copy link
Collaborator

Choose a reason for hiding this comment

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

why place it in this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't sure where to put it in order to avoid circular deps, but I figured it's only used for script payloads so just put it here

@xbtmatt xbtmatt force-pushed the transaction-arguments branch from bb85c02 to 336d764 Compare October 11, 2023 17:42
@0xmaayan 0xmaayan requested a review from a team October 11, 2023 17:45
@xbtmatt xbtmatt force-pushed the transaction-arguments branch from 336d764 to 3167861 Compare October 11, 2023 17:50
@xbtmatt xbtmatt enabled auto-merge (squash) October 11, 2023 17:51
@xbtmatt xbtmatt merged commit 4baec44 into main Oct 11, 2023
10 checks passed
@xbtmatt xbtmatt deleted the transaction-arguments branch October 11, 2023 17:53
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.

2 participants