Skip to content
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

[2/x][dev-inspect] Add authority end-point #6651

Merged
merged 7 commits into from
Dec 15, 2022

Conversation

tnowacki
Copy link
Contributor

@tnowacki tnowacki commented Dec 8, 2022

  • Add dev_inspect_transction entry point on the authority
  • Added new result type for dev inspect

Follow-up to #6538 that is blocked by move-language/move#728.

Putting this up initially to get feedback on the result type.

}

#[derive(Clone, Serialize, Deserialize, JsonSchema)]
pub struct SuiExecutionResult {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gegaowp or @Jordan-Mysten, not sure if you have feedback on the result here? I used SuiTypeTag with the BCS bytes of the value, but I'm not sure if something else would be easier to use.

Copy link
Contributor

@gegaowp gegaowp Dec 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are mutable_reference_outputs objects, if so is there a way we can map the values to objects, or arguments in case there can be mutations around the same SuiTypeTag?

SuiTypeTag for the type of return values sgtm, thanks for the quick move!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They could be anything, since this lets you call anything.

For example:

fun bump(counter: &mut u64, amount: u64) { *counter = *counter + u64 }

So in this case, you would get mutable_reference_outputs with (0, /* some bytes */, TypeTag::u64)

transaction_digest: TransactionDigest,
) -> Result<DevInspectResults, anyhow::Error> {
let (gas_status, input_objects) =
transaction_input_checker::check_transaction_input(&self.database, &transaction)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I can tell this implies that the transaction needs a valid gas object, and this still verifies the object ownership but for dev inspect we want to bypass those checks

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do yeah. I can try to fix that in this PR. I basically just want to check that all the objects exist. Would be nice to just strait up remove the gas logic too, but not sure if that is feasible. Any ideas?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we can make gas_payment on the transaction an Option<ObjectRef>, and just enforce it to be some for non dev-inspect transactions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A good suggestion, which gives me an idea! We are also looking at making gas_payment be a vector of ObjectRef (for a different work stream). Once we do that, we can make gas_payment optionally empty for the dev-inspect case? (though it might be tricky for the execution engine)

@vercel
Copy link

vercel bot commented Dec 9, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated
wallet-adapter ⬜️ Ignored (Inspect) Dec 15, 2022 at 10:33PM (UTC)

@tnowacki tnowacki marked this pull request as ready for review December 15, 2022 20:34
@tnowacki tnowacki enabled auto-merge (squash) December 15, 2022 22:36
@tnowacki tnowacki merged commit cb0dbe4 into MystenLabs:main Dec 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants