[sdks] new Pay transaction type for generic payment #4452

sblackshear · 2022-09-02T17:02:37Z

This API is intended to be the one-stop shop for any kind of payment: as input, take:

an arbitrary number of coins
an arbitrary number of recipient addresses
a vector of amounts (must be same length as # of recipient addresses)

and do the obvious thing: use the input coins to pay each recipient the desired amount.

This PR adds a new transaction type and thorough tests. It still needs to be hooked up to the CLI, TS SDK, etc.

gegaowp

lgtm.

not about this PR, here we have "transfer" and "transferSui" bc "transfer sui" is a native function, should we unify that and hide the impl under the hood?

gegaowp · 2022-09-02T17:04:44Z

crates/sui-types/src/messages.rs

+ /// The addresses that will receive payment
+ pub recipients: Vec<SuiAddress>,
+ /// The amounts each recipient will receive.
+ /// Must be the same length as amounts


as recipients ?

666lcz · 2022-09-02T20:59:01Z

crates/sui-types/src/messages.rs

+ /// The type of each object in `coins`
+ pub coin_type: TypeTag,


We don't really need this as it can be derived from the coins right?

It can indeed, and we can assume/enforce the vector will only include coins of the same type.

Yep, good point!

666lcz · 2022-09-02T21:11:16Z

crates/sui-types/src/messages.rs

+#[derive(Debug, PartialEq, Eq, Hash, Clone, Serialize, Deserialize)]
+pub struct Pay {
+ /// The coins to be used for payment
+ pub coins: Vec<ObjectRef>,


An edge case for this is whether the caller should include the gas object(used to pay for the transaction) in this vector or not if the payment is in SUI. I think the semantics could be if they pass in the gas object ref in coins(in most cases they should), then it will be used for both gas payment and the payment to recipients. Otherwise, the gas object will only be used for gas payment.

Pay is a SingleTransactionKind which I think should include and only include specs to construct this kind of txn, leaving gas payment to TransactionData, as TransactionDatahas bothgas_payment: ObjectRefandSingleTransactionKind`.
https://github.com/gegaowp/sui/blob/a300658ec5ad1126155982bc5ffad834466a37fc/crates/sui-types/src/messages.rs#L354

I think it is better this way because:

validity check of Pay is easy, we will just make sure sum(coin_balance) >= sum(amounts); otherwise, we will need to consider various gas cost, which is not guaranteed even with gas estimation.

the problem of "coin selection for payment" is isolated, and its solution can be used for all kinds of txns.

Yes, gas_payment should always be specified at the top level of TransactionData. My comment is more about whether we should allow the same object ref to appear in both TransactionData.gas_payment and Pay.coins.

yes, that's what I meant as well, above are advantages of using a separate gas_payment object instead of one in coins for gas payment, the "price" is that users will be blocked on an edge case when all coins they own cannot have a partition such that one can be used for gas payment, and the rest are used for Pay.

transferSui solves this edge case partially when user wants to transfer to one recipient while they only have one Sui coin. One reason it is relatively easy for transferSui to do so is that, the input gas_payment always exist thru the txn, see here
https://github.com/gegaowp/sui/blob/sui-call/crates/sui-core/src/execution_engine.rs#L215
but for Pay here, the original gas_payment object is very likely to be gone after merging & splitting for coins of amounts.

I also like the idea of removing transferSui as a SingleTransactionKind, it will make txn execution codes cleaner by removing the temporary_store.read_object. For the edge case, SDK & wallet can suggest a prerequisite txn before "submit" when needed

The intention is that Pay works for all coin types (since I think settlement in stablecoins will be common), but reusing the gas coin only applies for payments in SUI.

There could be a PaySui function that acts like Pay but allows you to reuse the gas coin, but I think that this (if it exists at all, which may or may not make sense) should probably be a separate tx type. This way, Pay won't have to worry about details like whether coin type is one that allows the gas coin to be debited for payments, whether the gas coin gets debited before or after the rest of the coins, and whether an empty list of coins is acceptable.

If we do want to support a PaySui function, I think eliminating TransferSui probably makes sense--just generalize it to accepting a (possibly-empty) vector of coins in addition to the gas coin, and a vec of addresses + amounts instead of a single address + amount.

666lcz · 2022-09-02T21:22:43Z

crates/sui-types/src/messages.rs

@@ -115,6 +129,8 @@ pub enum SingleTransactionKind {
 Call(MoveCall),
 /// Initiate a SUI coin transfer between addresses
 TransferSui(TransferSui),


TransferSui is just Pay with a single recipient and amount, we should consider removing TransferSui after we added Pay so that people won't be confused about which one to use.

The subtle difference between the two is that:

Pay works for any coin type

TransferSui can debit the gas coin, whereas Pay cannot (because Pay might be operating over a different coin type)

oxade · 2022-09-06T16:59:18Z

This is great @sblackshear
From our Slack convo:
I want to provide a more granular utility which simply creates N coins from M coins, all still belonging to the signer.
Now what the signer chooses to do with these coins (e.g transfer to recipient for Pay use-case) is up to them.
So it does not negate the need for Pay API, but the Pay logic can use it as a precursor to transfer N.

As a secondary step, I will look into making the cost for this logic is easily characterized so that users can estimate what a transformation would cost.

With this solution we will be able to power other use cases like coin mgmt, paying for a TX with multiple coins (transform M to 1 and then use it to pay), etc
cc @kchalkias

sblackshear · 2022-09-20T00:44:53Z

@Jordan-Mysten @666lcz @gegaowp this is (finally) ready for review.

I added a lot of goodies for unit testing that should hopefully make life much easier if we go forward with the plan to generalize TransferSui to PaySui (it should just be a wrapper around pay() that passes the gas coin as an extra arg).

Note that this PR just adds a new transaction type + tests. It still needs to be hooked up to the CLI, TS SDK, etc. I am not planning to take that on because I don't want to keep blocking folks by starting code I can't finish quickly...

gegaowp

tests LG! I can help with the transferSui and SDK changes, just a couple of nits.

gegaowp · 2022-09-20T00:58:28Z

crates/sui-core/src/execution_engine.rs

+ recipients,
+ amounts,
+ }) => {
+ let coin_objects = // unwrap is is safe because we built the object map from the transaction


gegaowp · 2022-09-20T01:02:40Z

crates/sui-types/src/messages.rs

+ /// The addresses that will receive payment
+ pub recipients: Vec<SuiAddress>,
+ /// The amounts each recipient will receive.
+ /// Must be the same length as amounts


This API is intended to be the one-stop shop for any kind of payment: as input, take: - an arbitrary number of coins - an arbitrary number of recipient addresses - a vector of amounts (must be same length as # of recipient addresses) and do the obvious thing: use the input coins to pay each recipient the desired amount.

lxfind · 2022-09-23T22:56:04Z

Question: how often do we expect this transaction kind to be used? Trying to understand why we didn't use Move call and instead choose to implement a native transaction.

sblackshear requested review from awelc, longbowlu, kchalkias and gegaowp September 2, 2022 17:02

gegaowp approved these changes Sep 2, 2022

View reviewed changes

666lcz reviewed Sep 2, 2022

View reviewed changes

oxade self-requested a review September 6, 2022 15:33

oxade mentioned this pull request Sep 8, 2022

Coin transformer utility #4528

Closed

sblackshear force-pushed the pay_tx_type branch from cc3b0e8 to 070e0d6 Compare September 8, 2022 19:27

gegaowp mentioned this pull request Sep 9, 2022

[Question] Need object version check before executing transactions through SDK? #4432

Open

sblackshear force-pushed the pay_tx_type branch from 070e0d6 to d1e2d1e Compare September 20, 2022 00:17

sblackshear marked this pull request as ready for review September 20, 2022 00:22

sblackshear requested a review from Jordan-Mysten September 20, 2022 00:22

sblackshear force-pushed the pay_tx_type branch from d1e2d1e to 0c8c638 Compare September 20, 2022 00:36

gegaowp approved these changes Sep 20, 2022

View reviewed changes

sblackshear force-pushed the pay_tx_type branch 2 times, most recently from 7812ec8 to d92fcac Compare September 21, 2022 16:06

sblackshear requested a review from huitseeker as a code owner September 21, 2022 16:06

sblackshear force-pushed the pay_tx_type branch from d92fcac to c7316af Compare September 21, 2022 19:45

sblackshear changed the title ~~[rfc] [sdks] new Pay transaction type for generic payment~~ [sdks] new Pay transaction type for generic payment Sep 21, 2022

sblackshear enabled auto-merge (rebase) September 21, 2022 20:38

sblackshear force-pushed the pay_tx_type branch from c7316af to b8b6872 Compare September 23, 2022 00:50

sblackshear merged commit 957f601 into MystenLabs:main Sep 23, 2022

gegaowp mentioned this pull request Sep 24, 2022

[SDK] add Pay to SDKs #4774

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[sdks] new Pay transaction type for generic payment #4452

[sdks] new Pay transaction type for generic payment #4452

sblackshear commented Sep 2, 2022 •

edited

Loading

gegaowp left a comment

gegaowp Sep 2, 2022

gegaowp Sep 20, 2022

666lcz Sep 2, 2022

kchalkias Sep 2, 2022

sblackshear Sep 3, 2022

666lcz Sep 2, 2022 •

edited

Loading

gegaowp Sep 2, 2022

666lcz Sep 2, 2022

gegaowp Sep 3, 2022 •

edited

Loading

sblackshear Sep 3, 2022

sblackshear Sep 3, 2022

666lcz Sep 2, 2022

sblackshear Sep 3, 2022

oxade commented Sep 6, 2022 •

edited

Loading

sblackshear commented Sep 20, 2022 •

edited

Loading

gegaowp left a comment

gegaowp Sep 20, 2022

gegaowp Sep 20, 2022

lxfind commented Sep 23, 2022

		/// The type of each object in `coins`
		pub coin_type: TypeTag,

[sdks] new Pay transaction type for generic payment #4452

[sdks] new Pay transaction type for generic payment #4452

Conversation

sblackshear commented Sep 2, 2022 • edited Loading

gegaowp 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

666lcz Sep 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gegaowp Sep 3, 2022 • 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

oxade commented Sep 6, 2022 • edited Loading

sblackshear commented Sep 20, 2022 • edited Loading

gegaowp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lxfind commented Sep 23, 2022

sblackshear commented Sep 2, 2022 •

edited

Loading

666lcz Sep 2, 2022 •

edited

Loading

gegaowp Sep 3, 2022 •

edited

Loading

oxade commented Sep 6, 2022 •

edited

Loading

sblackshear commented Sep 20, 2022 •

edited

Loading