MystenLabs / sui

Sui, a next-generation smart contract platform with high throughput, low latency, and an asset-oriented programming model powered by the Move programming language
https://sui.io
Apache License 2.0
6.04k stars 11.13k forks source link

Sui dApp Kit: useSignAndExecuteTransaction: Do we really need a Tralnsaction object to be instantiated for argument parsing/preparing functions #18527

Closed kkomelin closed 2 months ago

kkomelin commented 3 months ago

Problem/Motivation

In Sui dApp Starter, I have the useTransact hook, which works like this:

const { transact } = useTransact()

const prepareTransaction = (packageId: string, objectId: string, name: string) => {
  const tx = new Transaction()
  tx.moveCall({
    arguments: [tx.object(objectId), tx.pure.string(name), tx.object('0x8')],
    target: `${packageId}::greeting::set_greeting`,
  })
  return tx
}

transact(prepareTransaction(packageId, objectId, name))

The only reason I need the Transaction object to be instantiated outside of the useTransact hook is to be able to use tx.pure.string(), tx.object(), etc.

In order to further improve the DX of useSignAndExecuteTransaction and consequently useTransact, it would be good to be able to only pass the target and arguments to useTransact and not the already instantiated and configured transaction object like this:

const { transact } = useTransact()

transact(`${packageId}::greeting::set_greeting`, [Transaction.object(objectId), Transaction.pure.string(name), Transaction.object('0x8')])

Then it would be super-straightforward, like calling a regular function with arguments.

For that, it would be good to be able to use the argument parsing/preparing functions (object(), pure.string(), etc.) as static functions of the Transaction class, for example Transaction.object(objectId).

So my question is do we really need the Transaction object to be already instantiated to prepare the arguments?

hayes-mysten commented 3 months ago

The short answer is that currently the tx.pure and tx.object functions do need a transaction instance. This is because what these functions do is add an input to the transaction. What is returned is a reference to that inputs index in the transaction, and the returned value doesn't actually contain the original input value. Changing this isn't trivial, and would require carefully thinking through the implications.

In the 1.0 release, we added some features that might be helpful here. We probably can't trivially change the tx.object/tx.pure calls themselves, but creating methods for creating arguments outside of a transaction should be relatively easy. Commands can now take callbacks as arguments, where the callback is passed the transaction argument for when the command is passed to a Transaction.

Eg:

tx.moveCall({
    arguments: [(tx) => tx.object(objectId), (tx) => tx.pure.string(name), (tx) => tx.object('0x8')],
    target: `${packageId}::greeting::set_greeting`,
  })

This means we could probably add an Arguments object (similar to the Commands and Inputs objects) that work something like:

const Arguments = {
  object: (id: string) => (tx: Transaction) => tx.object(id),
  pure: /* mimmic tx.pure with the callback pattern */,
}

function transact(target: string, arguments: ((tx: Transaction) => TransactionArgument)[]) {
  const tx = new Transaction()
  tx.moveCall({ target, arguments })

  /* rest of transact */
}

transact(`${packageId}::greeting::set_greeting`, [Arguments.object(objectId), Arguments.pure.string(name), Arguments.object('0x8')])
hayes-mysten commented 3 months ago

here's an example of how this might work: https://github.com/MystenLabs/sui/pull/18540

kkomelin commented 3 months ago

Hey @hayes-mysten,

Thank you for explaining how the arguments work and for providing the alternative solution.

Not sure if this alternative solution can significantly simplify my useTransact hook for the end-user and for me also (I would need to maintain the list of supported methods of the Argument class in the long run), so I would probably stick with my previous version but it's very good to have another option, so thank you!

I think we can close the issue. Do you agree?

hayes-mysten commented 2 months ago

All the Argument methods would return(tx: Transaction) => TransactionArgument so, I wouldn't expect that you'd need to maintain a list of specific argument methods.

kkomelin commented 2 months ago

@hayes-mysten Thanks. It doesn't look simpler/more straightforward than current version, so I will rather stick to current one.

hayes-mysten commented 2 months ago

No worries, but isn't this exactly the API you were asking for (with a different name):

transact(`${packageId}::greeting::set_greeting`, [Transaction.object(objectId), Transaction.pure.string(name), Transaction.object('0x8')])

vs

transact(`${packageId}::greeting::set_greeting`, [Arguments.object(objectId), Arguments.pure.string(name), Arguments.object('0x8')])