[dev-inspect] Combine to a single end point #7372

tnowacki · 2023-01-13T00:14:40Z

Merge the transaction mode and move call only mode. Specifying the gas object isn't really necessary for dev inspect

I am not entirely sure how the read_api parts of the code should change. Maybe you can help with that @666lcz :) Thanks in advance!

vercel · 2023-01-13T00:14:44Z

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

4 Ignored Deployments

Name	Status	Updated
explorer	⬜️ Ignored (Inspect)	Jan 26, 2023 at 2:55AM (UTC)
explorer-storybook	⬜️ Ignored (Inspect)	Jan 26, 2023 at 2:55AM (UTC)
frenemies	⬜️ Ignored (Inspect)	Jan 26, 2023 at 2:55AM (UTC)
wallet-adapter	⬜️ Ignored (Inspect)	Jan 26, 2023 at 2:55AM (UTC)

patrickkuo · 2023-01-13T11:22:18Z

Is this same as dry_run_transaction minus the gas object? Do we still need dry_run_transaction after this? @gegaowp ?

amnn

Looks good from the authority/adapter side. Holding off on the accept to give @666lcz a chance to look at the API side. I'm guessing we'll need to make changes to the SDK(s) downstream of the RPC changes as well? Will leave it between you to navigate that.

crates/sui-open-rpc/spec/openrpc.json

crates/sui-types/src/messages.rs

crates/sui-core/src/authority.rs

amnn · 2023-01-13T13:54:36Z

crates/sui-core/src/unit_tests/authority_tests.rs

    assert!(effects.deleted.is_empty());
-    assert_eq!(effects.mutated[0].reference.object_id, gas_object_id);
+    assert!(effects.gas_used.computation_cost > 0);


Related to my question about the gas schedule, can we make any kind of claim that for a valid move call, if you send a dev-inspect transaction, and against exactly the same on-chain state, you send the move call, that their computation costs would match? (Presumably storage costs won't because we're not modifying the gas object?)

If we can, can we add a test to that effect? (Fine to do in a separate PR).

I think you should get the same result? Let me test it :)

Testing this btw opened up a bunch of issues now fixed. Sorry the PR is a bit drastically different after this

crates/sui-core/src/unit_tests/authority_tests.rs

crates/sui-types/src/gas.rs

tnowacki · 2023-01-17T22:42:01Z

Is this same as dry_run_transaction minus the gas object? Do we still need dry_run_transaction after this? @gegaowp ?

It is not. Dry run runs object ownership checks, entry rule checks, valid usage of Pure, etc. Dry run should be (long term) equivalent to running the exact transaction possibly with/without gas.

Dev inspect is a literally any transaction with any arguments.

Mentioned in the meeting this morning, but copying here for posterity. We can split that up in a few different ways, but from a client side, it might make sense to expose dev-inspect and dry-run with different flags

amnn

This looks good to me! Will accept to unblock, but cc @666lcz in case there are changes necessary on the SDK side

amnn · 2023-01-19T19:31:06Z

crates/sui-core/src/authority.rs

@@ -1085,26 +1085,44 @@ impl AuthorityState {
        SuiTransactionEffects::try_from(effects, self.module_cache.as_ref())
    }

+    /// The object ID for gas can be any object ID, even for an uncreated object


Does this comment still make sense? This function doesn't accept a gas object ID.

Nope, sorry, was from an intermediate version of this PR

amnn · 2023-01-19T19:40:59Z

crates/sui-core/src/authority.rs

        let shared_object_refs = input_objects.filter_shared_objects();

+        // TODO should we error instead for 0?


I'm a fan of the existing solution (picking a sensible default).

amnn · 2023-01-19T19:42:44Z

crates/sui-core/src/unit_tests/authority_tests.rs

@@ -320,13 +320,14 @@ async fn test_dev_inspect_object_by_bytes() {
        .unwrap()
        .contents()
        .to_vec();
+    // gas used should be the same
+    let actual_gas_used: SuiGasCostSummary = effects.gas_used.into();
+    assert_eq!(actual_gas_used, dev_inspect_gas_summary);


crates/sui-json-rpc/src/read_api.rs

gegaowp

Great move, the read_api part looks good per se, will need proper BCS encoding indeed for txn kind. Maybe we can further merge dry run and dev inspect to one.

crates/sui-json-rpc/src/read_api.rs

666lcz · 2023-01-20T22:20:45Z

crates/sui-json-rpc/src/read_api.rs

        Ok(self
            .state
-            .dev_inspect_move_call(sender_address, move_call, epoch)
+            .dev_inspect_transaction(sender_address, tx_kind, gas_price.unwrap_or(1), epoch)


we should use reference gas price here as the default. I'll make this change in my diff as well unless there's any concern

666lcz

@tnowacki I have added TS Support. However, one of the e2e test is failing with the following error:

test/e2e/dev-inspect.test.ts > Test dev inspect > Dev inspect transaction with PayAllSui
Error: Error dev inspect transaction with request type: Error: RPC Error: SUI payment transactions use first input coin for gas payment, but found a different gas object.
 ❯ JsonRpcProvider.devInspectTransaction src/providers/json-rpc-provider.ts:737:13
    735|       return resp;
    736|     } catch (err) {
    737|       throw new Error(
       |             ^
    738|         `Error dev inspect transaction with requ…
    739|       );

Due to this check

sui/crates/sui-types/src/messages.rs

Line 973 in cc0c5bf

SuiError::UnexpectedGasPaymentObject

. Should we bypass this for DevInspect?

666lcz · 2023-01-23T20:55:13Z

sdk/typescript/src/providers/json-rpc-provider.ts

+      // TODO: remove after 0.23.0 is deployed in both DevNet and TestNet
+      if (version?.major == 0 && version?.minor < 23) {
+        return this.devInspectTransactionDeprecated(sender, tx, epoch);


@tnowacki We are going to release a new DevNet on Jan 25th(Wednesday). If this is not merged before the DevNet release, then we would need to update version?.minor < 23 to version?.minor < 24.

When is the cut for 0.23?

- Merge the transaction mode and move call only mode. Specifying the gas object isn't really necessary for dev inspect

- Merge the transaction mode and move call only mode. Specifying the gas object isn't really necessary for dev inspect - The two dev inspect modes (transaction and Move call) have been merged into a single endpoint - Some transaction types have been disabled in dev inspect Co-authored-by: Chris Li <[email protected]>

sync with MystenLabs/sui#7372

tnowacki requested review from lxfind, 666lcz and gegaowp January 13, 2023 00:14

tnowacki force-pushed the di-simple branch from e88b5cd to 9f2e61e Compare January 13, 2023 02:42

tnowacki requested a review from amnn January 13, 2023 02:43

amnn reviewed Jan 13, 2023

View reviewed changes

amnn mentioned this pull request Jan 14, 2023

[gas] Move storage gas price from on-chain to rust #7394

Merged

tnowacki force-pushed the di-simple branch from 9f2e61e to 6d8c886 Compare January 19, 2023 17:08

amnn approved these changes Jan 19, 2023

View reviewed changes

666lcz reviewed Jan 19, 2023

View reviewed changes

crates/sui-json-rpc/src/read_api.rs Show resolved Hide resolved

gegaowp approved these changes Jan 20, 2023

View reviewed changes

666lcz reviewed Jan 20, 2023

View reviewed changes

vercel bot temporarily deployed to Preview – wallet-adapter January 21, 2023 17:09 Inactive

vercel bot deployed to Preview – explorer January 21, 2023 17:10 View deployment

vercel bot temporarily deployed to Preview – frenemies January 21, 2023 17:11 Inactive

666lcz suggested changes Jan 21, 2023

View reviewed changes

666lcz reviewed Jan 23, 2023

View reviewed changes

vercel bot deployed to Preview – explorer January 24, 2023 18:45 View deployment

vercel bot temporarily deployed to Preview – frenemies January 24, 2023 18:46 Inactive

vercel bot temporarily deployed to Preview – wallet-adapter January 24, 2023 18:46 Inactive

tnowacki force-pushed the di-simple branch from 8622d93 to 65a0020 Compare January 24, 2023 19:29

tnowacki and others added 6 commits January 25, 2023 16:30

[dev-inspect] Combine to a single end point

79952f5

- Merge the transaction mode and move call only mode. Specifying the gas object isn't really necessary for dev inspect

fixup! [dev-inspect] Combine to a single end point

71209dc

Fix Rust API arguments

73f3ef8

Add TS support

0e6d3aa

fixup! Add TS support

ed18cdd

Fixed bad rebase... whoops

44b2a4f

tnowacki added 3 commits January 25, 2023 16:30

fixup! Fixed bad rebase... whoops

f40956d

fixup! fixup! Fixed bad rebase... whoops

9503cf2

fixup! fixup! fixup! Fixed bad rebase... whoops

3dc1aea

tnowacki force-pushed the di-simple branch from 8614687 to 3dc1aea Compare January 26, 2023 00:32

666lcz approved these changes Jan 26, 2023

View reviewed changes

tnowacki added breaking change labels Jan 26, 2023

tnowacki enabled auto-merge (squash) January 26, 2023 02:47

fixup! fixup! fixup! fixup! Fixed bad rebase... whoops

45b60a3

tnowacki merged commit 72481e7 into MystenLabs:main Jan 26, 2023

tnowacki deleted the di-simple branch January 26, 2023 17:56

FrankC01 mentioned this pull request Jan 26, 2023

[maintenance] Tracking Sui defects and changes FrankC01/pysui#51

Closed

666lcz mentioned this pull request Jan 27, 2023

[feature] sui_devInspectTransaction foregivness #7770

Closed

Gkirito added a commit to coming-chat/go-sui-sdk that referenced this pull request Feb 3, 2023

🔨 refactor(client/client_call): remove sui_devInspectMoveCall

6db2b9e

sync with MystenLabs/sui#7372

0xJEth added a commit to 0xJEth/go-sui-sdk that referenced this pull request Aug 8, 2024

🔨 refactor(client/client_call): remove sui_devInspectMoveCall

852e5a5

sync with MystenLabs/sui#7372

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[dev-inspect] Combine to a single end point #7372

[dev-inspect] Combine to a single end point #7372

tnowacki commented Jan 13, 2023

vercel bot commented Jan 13, 2023 •

edited

Loading

patrickkuo commented Jan 13, 2023 •

edited

Loading

amnn left a comment

amnn Jan 13, 2023

tnowacki Jan 17, 2023

tnowacki Jan 19, 2023

tnowacki commented Jan 17, 2023

amnn left a comment

amnn Jan 19, 2023

tnowacki Jan 19, 2023

amnn Jan 19, 2023

amnn Jan 19, 2023

gegaowp left a comment

666lcz Jan 20, 2023 •

edited

Loading

666lcz left a comment

666lcz Jan 23, 2023

tnowacki Jan 24, 2023

		let shared_object_refs = input_objects.filter_shared_objects();

		// TODO should we error instead for 0?

[dev-inspect] Combine to a single end point #7372

[dev-inspect] Combine to a single end point #7372

Conversation

tnowacki commented Jan 13, 2023

vercel bot commented Jan 13, 2023 • edited Loading

patrickkuo commented Jan 13, 2023 • edited Loading

amnn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnowacki commented Jan 17, 2023

amnn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gegaowp left a comment

Choose a reason for hiding this comment

666lcz Jan 20, 2023 • edited Loading

Choose a reason for hiding this comment

666lcz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vercel bot commented Jan 13, 2023 •

edited

Loading

patrickkuo commented Jan 13, 2023 •

edited

Loading

666lcz Jan 20, 2023 •

edited

Loading