-
Notifications
You must be signed in to change notification settings - Fork 11.1k
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
[sdks] new Pay transaction type for generic payment #4452
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
crates/sui-types/src/messages.rs
Outdated
/// The addresses that will receive payment | ||
pub recipients: Vec<SuiAddress>, | ||
/// The amounts each recipient will receive. | ||
/// Must be the same length as amounts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as recipients
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
crates/sui-types/src/messages.rs
Outdated
/// The type of each object in `coins` | ||
pub coin_type: TypeTag, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't really need this as it can be derived from the coins
right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can indeed, and we can assume/enforce the vector will only include coins of the same type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, good point!
#[derive(Debug, PartialEq, Eq, Hash, Clone, Serialize, Deserialize)] | ||
pub struct Pay { | ||
/// The coins to be used for payment | ||
pub coins: Vec<ObjectRef>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 both
gas_payment: ObjectRefand
SingleTransactionKind`.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -115,6 +129,8 @@ pub enum SingleTransactionKind { | |||
Call(MoveCall), | |||
/// Initiate a SUI coin transfer between addresses | |||
TransferSui(TransferSui), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The subtle difference between the two is that:
Pay
works for any coin typeTransferSui
can debit the gas coin, whereasPay
cannot (becausePay
might be operating over a different coin type)
This is great @sblackshear 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 |
cc3b0e8
to
070e0d6
Compare
070e0d6
to
d1e2d1e
Compare
d1e2d1e
to
0c8c638
Compare
@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 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... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tests LG! I can help with the transferSui and SDK changes, just a couple of nits.
recipients, | ||
amounts, | ||
}) => { | ||
let coin_objects = // unwrap is is safe because we built the object map from the transaction |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: is is
crates/sui-types/src/messages.rs
Outdated
/// The addresses that will receive payment | ||
pub recipients: Vec<SuiAddress>, | ||
/// The amounts each recipient will receive. | ||
/// Must be the same length as amounts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
7812ec8
to
d92fcac
Compare
d92fcac
to
c7316af
Compare
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.
c7316af
to
b8b6872
Compare
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. |
This API is intended to be the one-stop shop for any kind of payment: as input, take:
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.