Add TransactionPayloadInner to TransactionPayload #16012
Conversation
⏱️ 28m total CI duration on this PR
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
vusirikala
requested review from
JoshLind,
msmouse,
lightmark,
grao1991,
gregnazario,
banool,
zekun000,
sasha8,
ibalajiarun,
davidiw,
movekevin,
wrwg,
vgao1996,
georgemitenkov and
0xmaayan
as code owners
vusirikala
removed request for
davidiw,
gregnazario,
msmouse,
ibalajiarun,
grao1991,
JoshLind,
sasha8,
banool,
vgao1996,
wrwg and
0xmaayan
vusirikala
requested review from
igor-aptos
and removed request for
georgemitenkov and
movekevin
vusirikala
force-pushed
the
satya/orderless_pr_rename
branch
from
77eba8c to
71dd506
Compare
vusirikala
changed the title
vusirikala
force-pushed
the
satya/orderless_pr_rename
branch
3 times, most recently
from
3d76266 to
7d1d1dc
Compare
| /// A new transaction payload format with support for versioning. | ||
| /// Contains an executable (script/entry function) along with extra configuration. | ||
| /// Once this new format is fully rolled out, above payload variants will be deprecated. | ||
| Payload(TransactionPayloadInner), |
There was a problem hiding this comment.
Sorry for my ignorance (I known there has been much discussion), but why didn't we instead change the outer Transaction type and add Transaction::UserTransactionV2(SignedUserTxnEnum)? Is it that we assume that will impose more changes across the board?
A fn Transaction::get_user_transaction() -> Option<SignedUserTxnEnum> can convert the old SignedTransaction to SignedUserTxnEnum::Legacy(SignedUserTransaction), for example, so that we don't need to deal with the difference between the new and old format everywhere.
types/src/transaction/mod.rs
Outdated
| } | ||
| } | ||
|
|
||
| pub fn payload_type(&self) -> String { |
There was a problem hiding this comment.
nit: maybe use Cow<'static, str>
There was a problem hiding this comment.
Why is Cow<'static, str> better here?
There was a problem hiding this comment.
So that we don't need to allocate memory every time we return "Multisig::unknown".to_string()
| ContractAddress(*module_id.address()) | ||
| } | ||
| }, | ||
| _ => Others, |
There was a problem hiding this comment.
let's try not to add blanket match -- in case you add new variants to enums and you miss updating one of the matches.
| TransactionPayload::Multisig(_) => "multisig".to_string(), | ||
| TransactionPayload::Payload(TransactionPayloadInner::V1 { |
There was a problem hiding this comment.
Shall we add a .get_executable() -> TransactionExecutable so that we don't need to repeat ourselves in all the matches?
(or TransactionExecutableRef<'a>)
| @@ -43,6 +43,7 @@ impl UseCaseAwareTransaction for SignedTransaction { | |||
| use UseCaseKey::*; | |||
|
|
|||
| match self.payload() { | |||
| // Question: MultiSig contains an entry function too. Why isn't it handled like the entry function? | |||
There was a problem hiding this comment.
https://aptos-org.slack.com/archives/C03N83P7QUC/p1707765924508469
the multisig payload can carry None (and IIUC it's pretty likely), so I chose to take the easy way.. -_-
types/src/transaction/multisig.rs
Outdated
| @@ -33,6 +34,16 @@ impl Multisig { | |||
| ), | |||
| } | |||
| } | |||
|
|
|||
| pub fn as_transaction_executable(&self) -> TransactionExecutable { | |||
| // TODO: See how to avoid cloning the entry function here. | |||
There was a problem hiding this comment.
do TransactionExecutableRef<'a>
vusirikala
force-pushed
the
satya/orderless_pr_rename
branch
7 times, most recently
from
c08e3d6 to
be2f08c
Compare
vusirikala
force-pushed
the
satya/orderless_pr_rename
branch
from
be2f08c to
a98b1e0
Compare
Description
How Has This Been Tested?
Key Areas to Review
Type of Change
Which Components or Systems Does This Change Impact?
Checklist