[JSONRPC] - Use a more structured dynamic field name instead of MoveValue::to_string #7318

patrickkuo · 2023-01-11T16:31:57Z

Current implementation of RPC dynamic field query use the MoveValue::to_string as name, which is quite ugly and hard to construct on client side to be used in sui_getDynamicFieldObject.

Example name struct in move:

    struct Name has copy, drop, store {
        name_str: std::string::String
    }

sui_getDynamicFields response:

{
    "jsonrpc": "2.0",
    "result": {
        "data": [
            {
                "name": "0xc03bf9390eb6849760890f4dabc53e64505c0159::object_basics::Name {name_str: 0x1::string::String {bytes: vector[84u8, 101u8, 115u8, 116u8, 32u8, 78u8, 97u8, 109u8, 101u8]}}",
                "type": "DynamicField",
                "objectType": "0xc03bf9390eb6849760890f4dabc53e64505c0159::object_basics::Object",
                "objectId": "0x3f8af3b8158af4b25d51c7d39c7c514cea94b1d0",
                "version": 10727,
                "digest": "AeSblVNBG3z6hTjwzwYE5rbUpEcjCEXzOUEt5rObkHs="
            }
        ],
        "nextCursor": null
    },
    "id": 1
}

This PR change the String name to DynamicFieldName, which is a struct of type and json value.

pub struct DynamicFieldName {
    pub type_: TypeTag,
    pub value: Value,
}

The name field become more readable and easier to create on client side.

{
    "jsonrpc": "2.0",
    "result": {
        "data": [
            {
                "name": {
                    "type": "0x1a43c696e91e24e8128c6764e713d38d24ffe4e1::object_basics::Name",
                    "value": {
                        "name_str": "Test Name"
                    }
                },
                "type": "DynamicField",
                "objectType": "0x1a43c696e91e24e8128c6764e713d38d24ffe4e1::object_basics::Object",
                "objectId": "0x7fe687114a592a8ae5baecf5954a3d0d2a74c60b",
                "version": 5,
                "digest": "fOmZ+NrNl2BP5MT1aGJ+YUk1rpiAmF5Bg6qGGbcgaIw="
            }
        ],
        "nextCursor": null
    },
    "id": 1
}

Example response for primitive type name:

{
    "jsonrpc": "2.0",
    "result": {
        "data": [
            {
                "name": {
                    "type": "bool",
                    "value": true
                },
                "type": "DynamicField",
                "objectType": "0x1a43c696e91e24e8128c6764e713d38d24ffe4e1::object_basics::Object",
                "objectId": "0x33274b734d787c19fc19871fb7b8c51a048b58b0",
                "version": 8,
                "digest": "56q6L9uzK4OswbZ9VvQak4CdOrhwA5DiweXrcA44JWA="
            }
        ],
        "nextCursor": null
    },
    "id": 1
}

vercel · 2023-01-11T16:32:03Z

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

Name	Status	Preview	Comments	Updated
explorer	✅ Ready (Inspect)	Visit Preview	💬 Add your feedback	Feb 22, 2023 at 11:59AM (UTC)
explorer-storybook	✅ Ready (Inspect)	Visit Preview	💬 Add your feedback	Feb 22, 2023 at 11:59AM (UTC)
frenemies	✅ Ready (Inspect)	Visit Preview	💬 Add your feedback	Feb 22, 2023 at 11:59AM (UTC)
wallet-adapter	✅ Ready (Inspect)	Visit Preview	💬 Add your feedback	Feb 22, 2023 at 11:59AM (UTC)

patrickkuo · 2023-01-18T22:35:22Z

friendly nudge :) this PR need some reviews

tnowacki

Things LGTM but I feel like someone more familiar with the crate might want to take a look at sui_serde changes.

Happy to stamp, just have a question about test coverage :)

tnowacki · 2023-01-19T00:08:48Z

crates/sui-core/src/unit_tests/data/object_basics/sources/object_basics.move

+    public entry fun add_field_with_struct_name(o: &mut Object, v: Object) {
+        sui::dynamic_field::add(&mut o.id, Name {name_str: std::string::utf8(b"Test Name")}, v);
+    }


Should we also add dynamic_object_field tests for this case? The process to retrieve the inner name will be a tad different because of the wrapper.

sure I will add tests for dynamic_object_field

joyqvq · 2023-01-25T14:57:32Z

crates/sui-types/src/crypto.rs

@@ -864,7 +865,7 @@ impl Signer<Signature> for Secp256k1KeyPair {
 #[derive(Clone, Debug, Serialize, Deserialize, JsonSchema, PartialEq, Eq, Hash)]
 pub struct Secp256r1SuiSignature(
    #[schemars(with = "Base64")]
-    #[serde_as(as = "Readable<Base64, Bytes>")]
+    #[serde_as(as = "Readable<ToArray<Base64>, Bytes>")]


what does this ToArray do?

ah i see... this is to use the sized array, why do we need it? (context is im probably going to replace this with a fastcrypto trait to do just this in a later PR)

ya we need this because fast crypto's Base64 only support serde_as Vec<u8>

this is used so I don't have to make changes in fast crypto 🙃, we can remove this once serde_as array is supported

666lcz · 2023-01-28T07:41:30Z

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

@@ -148,14 +149,14 @@ impl RpcReadApiServer for ReadApi {
    async fn get_dynamic_field_object(
        &self,
        parent_object_id: ObjectID,
-        name: String,
+        name: DynamicFieldName,


We also need to update https://github.com/MystenLabs/sui/blob/main/sdk/typescript/src/providers/json-rpc-provider.ts#L943 . I am happy to work on this by early next week

damirka · 2023-01-31T10:37:33Z

@patrickkuo any updates on this one? I believe it can be related to the #7668 implementation.

patrickkuo · 2023-01-31T10:44:38Z

@patrickkuo any updates on this one? I believe it can be related to the #7668 implementation.

I will update the TS SDK (this PR has been opened for too long, TS SDK support was added recently 🤣), will chase for approval after this.

patrickkuo · 2023-02-21T14:39:04Z

Moved some serde changes to fastcrypto, and fixed TS failing tests, this PR is ready for more reviews, @tnowacki @666lcz

sblackshear · 2023-02-21T23:13:01Z

crates/sui-types/src/dynamic_field.rs

+    // Bincode does not like serde_json::Value, rocksdb will not insert the value without serializing value as string.
+    #[schemars(with = "Value")]
+    #[serde_as(as = "Readable<_, DisplayFromStr>")]
+    pub value: Value,


It seems a bit json-rpc specific to use a json::Value here--someone looking at a DynamicFieldName from inside Rust would presumably want to see a MoveStruct or something else a bit more structured

I thought MoveStruct is quite hard to construct in rust. Currently user can use the serde_json's macro json!{} to construct the value, it is not structured but quite convenient. Maybe I can add something in the Rust SDK to accept more structured MoveValue input.

sblackshear · 2023-02-21T23:14:05Z

crates/sui-types/src/dynamic_field.rs

+pub struct DynamicFieldName {
+    #[schemars(with = "String")]
+    #[serde_as(as = "Readable<DisplayFromStr, _>")]
+    pub type_: TypeTag,


We use a TypeTag here, but a String for object_type above. Do we also want to change object_type in the future?

object_type should use TypeTag as well, I will change it now to make it consistent.

…f MoveValue::to_string

added serde adaptors to make fastcrypto's Hex, Base64 and Base58 work with array and TryFrom<Vec<u8>> types.

patrickkuo force-pushed the pat/dynamic_field_parse_name branch 2 times, most recently from 8e8a54a to b07cab5 Compare January 12, 2023 10:52

patrickkuo marked this pull request as ready for review January 12, 2023 12:38

patrickkuo requested review from joyqvq, kchalkias and benr-ml as code owners January 12, 2023 12:38

patrickkuo requested review from Jordan-Mysten, sblackshear, tnowacki and siomari January 12, 2023 12:38

FrankC01 mentioned this pull request Jan 12, 2023

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

Closed

patrickkuo force-pushed the pat/dynamic_field_parse_name branch from b2a0a62 to 65b3630 Compare January 18, 2023 14:50

patrickkuo requested a review from damirka January 18, 2023 14:54

tnowacki reviewed Jan 19, 2023

View reviewed changes

patrickkuo force-pushed the pat/dynamic_field_parse_name branch 2 times, most recently from ff3c02b to 0762bd0 Compare January 24, 2023 16:27

joyqvq reviewed Jan 25, 2023

View reviewed changes

patrickkuo force-pushed the pat/dynamic_field_parse_name branch from 0762bd0 to 1a49324 Compare January 27, 2023 10:32

666lcz reviewed Jan 28, 2023

View reviewed changes

patrickkuo force-pushed the pat/dynamic_field_parse_name branch from 1a49324 to 2c6865c Compare January 31, 2023 12:31

vercel bot temporarily deployed to Preview – frenemies January 31, 2023 12:32 Inactive

vercel bot deployed to Preview – explorer January 31, 2023 12:32 View deployment

vercel bot temporarily deployed to Preview – wallet-adapter January 31, 2023 12:32 Inactive

patrickkuo requested a review from 666lcz January 31, 2023 13:07

patrickkuo force-pushed the pat/dynamic_field_parse_name branch from 2cb8d2b to 2e7eef8 Compare February 8, 2023 23:41

vercel bot temporarily deployed to Preview – wallet-adapter February 8, 2023 23:42 Inactive

vercel bot temporarily deployed to Preview – frenemies February 21, 2023 13:59 Inactive

vercel bot deployed to Preview – explorer February 21, 2023 13:59 View deployment

patrickkuo requested a review from tnowacki February 21, 2023 14:37

sblackshear approved these changes Feb 21, 2023

View reviewed changes

patrickkuo added 10 commits February 22, 2023 10:59

use MoveValue type + parsed json value for DynamicFieldName instead o…

127c17a

…f MoveValue::to_string

add more name parsing test

e6f1931

make Readable generic

250acd5

added serde adaptors to make fastcrypto's Hex, Base64 and Base58 work with array and TryFrom<Vec<u8>> types.

use TypeTag instead of String

ae01de7

add test for dynamic_object_field and bug fix

247f380

add DynamicFieldName to TS SDK

d34a522

move Hex deserialize as array to fastcrypto

c324680

fixup after rebase

1a8de24

add backward compatibility

b48e50d

fix ts

2d6d9da

patrickkuo force-pushed the pat/dynamic_field_parse_name branch from aabc0ac to 2d6d9da Compare February 22, 2023 11:00

vercel bot temporarily deployed to Preview – wallet-adapter February 22, 2023 11:00 Inactive

vercel bot deployed to Preview – explorer February 22, 2023 11:01 View deployment

vercel bot temporarily deployed to Preview – frenemies February 22, 2023 11:01 Inactive

vercel bot temporarily deployed to Preview – explorer-storybook February 22, 2023 11:01 Inactive

fix TS failing test

271452b

vercel bot temporarily deployed to Preview – wallet-adapter February 22, 2023 11:57 Inactive

vercel bot deployed to Preview – explorer February 22, 2023 11:58 View deployment

vercel bot temporarily deployed to Preview – explorer-storybook February 22, 2023 11:58 Inactive

vercel bot temporarily deployed to Preview – frenemies February 22, 2023 11:59 Inactive

patrickkuo merged commit 6ff0c78 into main Feb 22, 2023

patrickkuo deleted the pat/dynamic_field_parse_name branch February 22, 2023 12:40

GrapeBaBa mentioned this pull request Mar 5, 2023

Tracking sui 0.28 breaking changes GrapeBaBa/sui4j#94

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JSONRPC] - Use a more structured dynamic field name instead of MoveValue::to_string #7318

[JSONRPC] - Use a more structured dynamic field name instead of MoveValue::to_string #7318

patrickkuo commented Jan 11, 2023 •

edited

Loading

vercel bot commented Jan 11, 2023 •

edited

Loading

patrickkuo commented Jan 18, 2023

tnowacki left a comment

tnowacki Jan 19, 2023

patrickkuo Jan 23, 2023

joyqvq Jan 25, 2023

joyqvq Jan 25, 2023

patrickkuo Jan 25, 2023 •

edited

Loading

patrickkuo Jan 25, 2023

666lcz Jan 28, 2023 •

edited

Loading

damirka commented Jan 31, 2023

patrickkuo commented Jan 31, 2023

patrickkuo commented Feb 21, 2023

sblackshear Feb 21, 2023

patrickkuo Feb 22, 2023

sblackshear Feb 21, 2023

patrickkuo Feb 22, 2023

[JSONRPC] - Use a more structured dynamic field name instead of MoveValue::to_string #7318

[JSONRPC] - Use a more structured dynamic field name instead of MoveValue::to_string #7318

Conversation

patrickkuo commented Jan 11, 2023 • edited Loading

vercel bot commented Jan 11, 2023 • edited Loading

patrickkuo commented Jan 18, 2023

tnowacki 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

patrickkuo Jan 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

666lcz Jan 28, 2023 • edited Loading

Choose a reason for hiding this comment

damirka commented Jan 31, 2023

patrickkuo commented Jan 31, 2023

patrickkuo commented Feb 21, 2023

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

patrickkuo commented Jan 11, 2023 •

edited

Loading

vercel bot commented Jan 11, 2023 •

edited

Loading

patrickkuo Jan 25, 2023 •

edited

Loading

666lcz Jan 28, 2023 •

edited

Loading