Add withdraw reservation to Transaction #22434
Conversation
lxfind
requested review from
amnn,
wlmyng,
stefan-mysten and
emmazzz
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
2 Skipped Deployments
|
lxfind
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
lxfind
force-pushed
the
xun/accumulator-transaction-add-withdraw
branch
from
7295459 to
c617989
Compare
lxfind
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
lxfind
force-pushed
the
xun/accumulator-transaction-add-withdraw
branch
from
c617989 to
f2690c1
Compare
lxfind
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
lxfind
force-pushed
the
xun/accumulator-transaction-add-withdraw
branch
from
f2690c1 to
baa3f9e
Compare
lxfind
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
lxfind
force-pushed
the
xun/accumulator-transaction-add-withdraw
branch
from
baa3f9e to
f3410c9
Compare
lxfind
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
crates/sui-types/src/transaction.rs
Outdated
| pub struct BalanceWithdrawArg { | ||
| /// The maximum amount of the balance to withdraw. | ||
| /// If None, reserve the entire balance. | ||
| pub max_amount: Option<u64>, | ||
| /// The source of the balance to withdraw. | ||
| pub withdraw_from: WithdrawFrom, | ||
| } | ||
|
|
||
| #[derive(Debug, PartialEq, Eq, Hash, Clone, Serialize, Deserialize)] | ||
| pub enum WithdrawFrom { | ||
| /// Withdraw from the sender of the transaction, with balance type T as T in Balance<T>. | ||
| Sender(TypeTag), | ||
| // TODO: Add more options here, such as Sponsor. | ||
| } |
There was a problem hiding this comment.
do we want the type T to be hoisted into BalanceWithdrawArg? I'd imagine this is something that would exist in all variant here so it probably makes more sense to hoist it out
There was a problem hiding this comment.
So, that depends how powerful we want to make sponsored transactions to be.
Today a sponsored transaction can only sponsor SUI, which can be used for both gas payment and money transfer.
With address balances, indeed we could open up to allow sponsoring arbitrary Balance type T other than Sui beyond gas payment. I was initially thinking that we should keep Sponsor semantically identical to what we have today (i.e. besides gas sponsorship, there can be a separate withdraw reservation that sponsors some extra SUI)
crates/sui-types/src/transaction.rs
Outdated
| pub struct BalanceWithdrawArg { | ||
| /// The maximum amount of the balance to withdraw. | ||
| /// If None, reserve the entire balance. | ||
| pub max_amount: Option<u64>, |
There was a problem hiding this comment.
Should we just make an enum here? Fine with the option though
crates/sui-types/src/transaction.rs
Outdated
| /// Withdraw from the sender of the transaction, with balance type T as T in Balance<T>. | ||
| Sender(TypeTag), |
There was a problem hiding this comment.
This should be TypeInput and I think we should move the TypeInput onto the arg itself. I don't really see a case where the type specifier wont' be necessary
There was a problem hiding this comment.
(also TypeInput not TypeTag)
There was a problem hiding this comment.
Why do we need TypeInput now?
| CallArg::BalanceWithdraw(_) => { | ||
| todo!("Load balance withdraw call arg") | ||
| } |
There was a problem hiding this comment.
As a heads up, we are actively in the process of trying to get this code up and running. So let's be mindful of that as the downstream effects of having to deal with this now rather than later might make @tzakian and I cry many tears
There was a problem hiding this comment.
I don't need this piece of code working right now. I only need the CallArg to be visible in a transaction so that I can test the scheduler logic.
| @@ -113,6 +116,22 @@ pub enum ObjectArg { | |||
| Receiving(ObjectRef), | |||
| } | |||
|
|
|||
| #[derive(Debug, PartialEq, Eq, Hash, Clone, Serialize, Deserialize)] | |||
| pub struct BalanceWithdrawArg { | |||
There was a problem hiding this comment.
I think we should settle on a name for the module before naming this call arg (or just be prepared to rename it)
There was a problem hiding this comment.
Happy to rename to whatever
lxfind
force-pushed
the
xun/accumulator-transaction-add-withdraw
branch
from
f3410c9 to
6d14a07
Compare
lxfind
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
lxfind
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
lxfind
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
Description
Add a new CallArg to represent balance withdraw reservation.
This is not necessarily the final form. Adding it to support e2e testing and development.
Since this is all gated behind a protocol config, no transactions with withdraw reservation can be committed on-chain right now.
Test plan
CI
Release notes
Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.
For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.