-
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
Execution mode for dev-inspect #6538
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.
Like this approach a lot!
I think the ability to accept raw object bytes in dev_inspect
should be a boon for fuzzing.
crates/sui-adapter/src/adapter.rs
Outdated
match obj_arg { | ||
ObjectArg::ImmOrOwnedObject((id, _, _)) | ||
| ObjectArg::SharedObject { id, .. } => { | ||
let obj = state_view.read_object(id); | ||
assert_invariant!( | ||
obj.is_some(), | ||
format!("Object {} does not exist yet", id) | ||
); | ||
objects.insert(*id, obj.unwrap()); | ||
} | ||
} |
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: This is an irrefutable pattern, right? Could it be turned into a let
with pattern, to make that clear?
Granted you will need to parenthesise the pattern, so maybe it's not a readability win overall.
match obj_arg { | |
ObjectArg::ImmOrOwnedObject((id, _, _)) | |
| ObjectArg::SharedObject { id, .. } => { | |
let obj = state_view.read_object(id); | |
assert_invariant!( | |
obj.is_some(), | |
format!("Object {} does not exist yet", id) | |
); | |
objects.insert(*id, obj.unwrap()); | |
} | |
} | |
let ( | |
ObjectArg::ImmOrOwnedObject((id, _, _)) | | |
ObjectArg::SharedObject{ id, .. } | |
) = obj_arg; | |
let obj = state_view.read_object(id); | |
assert_invariant!( | |
obj.is_some(), | |
format!("Object {} does not exist yet", id) | |
); | |
objects.insert(*id, obj.unwrap()); | |
} |
+1, I like this design! We should also loop in @666lcz to look at how to accommodate the RPC changes in the SDK(s). |
Hey @amnn the context is that I had a half-done PR to implement dryRunMoveCall in RPC & SDK, and we realized that without changes like this, its usage is very limited. After this PR is merged, I will resume that work there and use this new transaction type to expose it to RPC and SDK. |
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.
apologies for the late reply as I was rushing the indexer work.
This is really neat! Left some comments re dryRunTx <> dryRunMoveCall
crates/sui-json-rpc-types/src/lib.rs
Outdated
effects: SuiTransactionEffects, | ||
/// Execution results (including return values) from executing the transactions | ||
/// Currently contains only return values from Move calls | ||
results: Result<Vec<(usize, ExecutionResult)>, String>, |
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.
amazing that we now have return values!
crates/sui-core/src/authority.rs
Outdated
@@ -1111,6 +1112,34 @@ impl AuthorityState { | |||
SuiTransactionEffects::try_from(effects, self.module_cache.as_ref()) | |||
} | |||
|
|||
pub async fn dev_inspect_transaction( | |||
&self, | |||
transaction: TransactionData, |
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.
TransactionData
requires gas payments info like gas payment object and budget, which seems not necessary for dryRunMoveCall. iiuc we will expose 2 endpoints:
- dryRunTxn, for which
dev_inspect_transaction
is perfect - dryRunMoveCall, where it would be great to have a function like below for a single move call without txn wrapper
fn dev_inspect_move_call(&self, move_call: MoveCall) -> Result<ExecutionResult>
@sblackshear any wisdom here?
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.
I think that seems reasonable here. Maybe we can even land this PR and iterate on the way the endpoints are exposed?
FWIW, I'm trying to avoid the term "dry run" here, hence the new term "dev inspect". I think we should still have dry run, but it should work literally exactly the same as the normal execution (except for committing effects). So that end point would mostly be used for sanity checking and gas estimation.
Whereas "dev inspect" would be a much more powerful tool that could be used for all sorts of things, but the developer needs to be aware that they might not actually be able to run a similar 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.
Agreed that this sounds reasonable!
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.
Re if we want both dryRunTxn and devInspectTxn, I think devInspectTxn is a better impl. and thus I would prefer to have one public RPC endpoint instead of 2.
I am neutral about the naming, either keeping dryRun or use devInspect sounds good to me, as long as this prefix is applied to both txn execution and move call.
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 3 types of transactions I see:
- Normal. Used for making state changes to the system
- Dry Run. Exactly the same as Normal, but does not make state changes. Useful for testing gas or testing the changes a transaction will make
- Dev Inspect. The wild west of RPC endpoints. Will let you call any Move function with just any arbitrary values.
The reason I think we should have Dry Run in addition is that not all "Dev Inspect" transactions can be run as Normal transactions. So you might end up thinking your txn will cost X gas, and you go and submit it and it gets rejected with some small gas payment. In other words, I think for the best developer experience, we need some endpoint that behaves just like a "Normal" transaction, but without committing changes.
- Introduces dev-inspect transaction type that can call any Move function - Allows return values from that Move function - Also provides much more detailed error message information
I updated the PR to be just the execution mode. The new APIs in authority will come in a later PR |
Attempting to address issues raised in #5836 and #4967.
I added a new
ExecutionMode
trait for the adapter and execution_engine that does a few things:Pure
call arg(Note: The arbitrary function and arbitrary arg are both controlled by a single flag currently)
While I'm not the biggest fan of this trait based approach, it feels cleaner than passing in some flags and then having asserts everywhere for the normal mode.
There are two execution modes:
Normal
andDevInspect
In the
Normal
execution mode, the flag is set to false and the handling of return values is skipped (just passes around unit()
).In the
DevInspect
execution mode, the flag is true (allowing arbitrary functions to be called with and arbitrary bytes) and the return values are passed back usingSerializedReturnValues
from the Move VM.I also hope we can provide the full
ExecutionError
when using the new endpoint, this will give folks the full error message (with stack traces and everything) from the Move VM!The PR does not currently work, and I will likely need the most help/feedback from @gegaowp and @Jordan-Mysten on actually creating an end point (lots of issues right now with what I'm doing and with the JsonSchema trait).