Optimize computation and memory usage by replace indexmap

[howjmay]

Description

BuilderArg is not necessary for the current codebase. It introduces extra wasted computation, and occupied more spaces. We can replace it with IndexSet of CallArg

Test plan

I ran the sui-types tests already

---

Release notes

Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.

For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.

• [ ] Protocol:
• [ ] Nodes (Validators and Full nodes):
• [ ] Indexer:
• [ ] JSON-RPC:
• [ ] GraphQL:
• [ ] CLI:
• [x] Rust SDK:

[vercel[bot]]

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

Name	Status	Preview	Comments	Updated (UTC)
sui-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	May 23, 2024 9:10pm

[vercel[bot]]

@howjmay is attempting to deploy a commit to the Mysten Labs Team on Vercel.

A member of the Team first needs to authorize it.

[howjmay]

Thank you @amnn and @tnowacki Sorry for misunderstanding the behavior here, and opened the PR. Maybe I can amend this PR into adding the explanation that @tnowacki you wrote. The current comment of force_separate_pure() is not that clear for me.

One thing I am still confused about the above example is

once they are used mutably the type is fixed and they cannot be recast, even if the bytes are the same. I thought the types of vectors have to be decided when they are passed. I am confused about how can we recast the types here.

[amnn]

Hi @howjmay, yes that is indeed a good idea!

Regarding your question: yes, the type of a given vector in Move or from a PTB MakeMoveVec command is fixed and cannot be recast, but a Pure Input is just bytes, so in cases where those bytes are not used mutably in the PTB, they can be used to represent multiple types (for example the bytes of a u64 could also represent two u32s).

The use of an IndexMap with a Pure key ensures that immutable usages with the same bytes do get deduplicated, while the other Pure key ensures that they don't.

Perhaps in addition to adding the comment, you could also update the names of the enum variants used as keys to the IndexMap to make it clear that one is for mutable Pure inputs (PureMut?) and one is for immutable (Pure or PureImmut)? Thoughts @tnowacki ?

[howjmay]

@amnn thank you for your thoughtful explanation. Is there any existing example I can see for the usage of recast like this in the PTB? I still one thing that I am curious about. The parameter will be converted into a vector of Argument and then encoded into bcs format which explicitly encodes the size of the vector. I think there should be a func to recast? Otherwise by accessing a specific element, and passing to a fixed move function interface, I don't know which step can do the reacsting.

Sorry for the question.

[amnn]

No problem, happy to answer these questions, thanks for sticking with it and contributing back as well :)

To see an example of the recasting happen, create a transaction for dev inspect that calls a function taking a &u64 parameter and a function taking a struct by reference, where that struct has two u32 fields. If both these calls have the same representation in BCS, they will be merged together.

The transaction will eventually get serialised into BCS, but an argument for a given command is an index into the inputs or results, and it's the inputs that contain the BCS representation of the Pure arguments.

[howjmay]

Thank you for the explanation. I am trying to build a test to verify it. It will take some time. Sorry I can't respond right now

[tnowacki]

Perhaps in addition to adding the comment, you could also update the names of the enum variants used as keys to the IndexMap to make it clear that one is for mutable Pure inputs (PureMut?) and one is for immutable (Pure or PureImmut)? Thoughts @tnowacki ?

I kind of prefer the more explicit naming that is there today. That isn't to say the naming can't be improved, but PureMut and PureImmut don't sit right with me, because we have no way of actually checking or knowing how the pure arguments are used, because we do not know the signatures of any function being called. We can only force, or not force, uniqueness of the arguments.

[howjmay]

Hi @amnn I have done some experiment, but I am confused about how to interpret the response of dev_inspect_transaction_block(). How can I see the intermediate arguments by dev_inspect_transaction_block()?

The testing move contract

/// Module: recast
module recast::recast {
    use sui::event;

    public struct U32_VALS has copy, drop {
        f0: u32,
        f1: u32,
    }

    public struct EventHere has copy, drop {
        u64_val: u64,
        u32_vals: U32_VALS,        
    }

    public fun create_container(): U32_VALS {
        U32_VALS {
            f0: 10,
            f1: 10,
        }
    }

    public fun try_recast(u64_val: &u64, u32_vals: &U32_VALS) {
        event::emit(EventHere {
            u64_val: *u64_val,
            u32_vals: *u32_vals,            
        })
    }
}

The rust client

let keypair = SuiKeyPair::Ed25519(Ed25519KeyPair::generate(&mut StdRng::from_seed([0; 32])));

    let public_key = keypair.public();
    let sender_addr = SuiAddress::from(&public_key);

    // Sui local network
    let sui_client = SuiClientBuilder::default()
        .build("https://fullnode.testnet.sui.io:443")
        .await?;

    let package_id = PACKAGE_ID_CONST.parse()?;

    // Get all my own objects
    let coins_response = &sui_client
        .read_api()
        .get_owned_objects(
            sender_addr,
            Some(SuiObjectResponseQuery::new_with_options(
                SuiObjectDataOptions::new().with_type(),
            )),
            None,
            None,
        )
        .await?;

    // Find a coin to use
    let coin = coins_response
        .data
        .iter()
        .find(|obj| obj.data.as_ref().unwrap().is_gas_coin())
        .unwrap();
    let coin = coin.data.as_ref().unwrap();

    let mut ptb = ProgrammableTransactionBuilder::new();

    let arg_u32 = ptb.command(Command::move_call(
        package_id,
        Identifier::new("recast")?,
        Identifier::new("create_container")?,
        vec![],
        vec![],
    ));

    let arg_u64 = ptb.pure(42949672970 as u64).expect("pure");
    ptb.command(Command::move_call(
        package_id,
        Identifier::new("recast")?,
        Identifier::new("try_recast")?,
        vec![],
        vec![arg_u64, arg_u32],
    ));

    let builder = ptb.finish();

    // Build the transaction data

    let gas_budget = 10_000_000;
    let gas_price = sui_client.read_api().get_reference_gas_price().await?;

    // create the transaction data that will be sent to the network
    let tx_data = TransactionData::new_programmable(
        sender_addr,
        vec![coin.object_ref()],
        builder,
        gas_budget,
        gas_price,
    );

    let tx_data_kind = tx_data.kind();
    let dev_inspect_res = sui_client
        .read_api()
        .dev_inspect_transaction_block(sender_addr, tx_data_kind.clone(), None, None, None)
        .await
        .expect("");

[amnn]

Sorry for the delay in getting back to you @howjmay. The call to dev_inspect_transaction_block will return an instance of this type:

https://github.com/MystenLabs/sui/blob/c6ba40b65185defcaef45c6a61ba06f9ba2458e9/crates/sui-json-rpc-types/src/sui_transaction.rs#L1160-L1182

The results field of this type includes the intermediate return values of individual transaction commands in the PTB.

[howjmay]

Hi @amnn thanks for explaining. With the aforementioned move contract, the following result is returned

{"jsonrpc":"2.0","result":{"effects":{"messageVersion":"v1","status":{"status":"success"},"executedEpoch":"77","gasUsed":{"computationCost":"1000000","storageCost":"988000","storageRebate":"0","nonRefundableStorageFee":"0"},"modifiedAtVersions":[{"objectId":"0xbba455e42d563393d97f9940dd30adc06ec14b5ec2e7a1b90e911e4dbc2a82f2","sequenceNumber":"1"}],"transactionDigest":"CuGC3z2x6SFhgRrkkEcN6AmrhaXNfmxgpEan7xVx1Kru","mutated":[{"owner":{"AddressOwner":"0x786dff8a4ee13d45b502c8f22f398e3517e6ec78aa4ae564c348acb07fad7f50"},"reference":{"objectId":"0xbba455e42d563393d97f9940dd30adc06ec14b5ec2e7a1b90e911e4dbc2a82f2","version":2,"digest":"F6cEgHtWLdBzSPzUDsukLMi8fKZuZv9HhvD4E3QZfqBm"}}],"gasObject":{"owner":{"AddressOwner":"0x786dff8a4ee13d45b502c8f22f398e3517e6ec78aa4ae564c348acb07fad7f50"},"reference":{"objectId":"0xbba455e42d563393d97f9940dd30adc06ec14b5ec2e7a1b90e911e4dbc2a82f2","version":2,"digest":"F6cEgHtWLdBzSPzUDsukLMi8fKZuZv9HhvD4E3QZfqBm"}},"eventsDigest":"FhcXK6DButmM46z59YwmZKmiHXpVfuaLvY1LrLAQkWRk","dependencies":["BWRdnJFp4jYu7UUcVrLhGR7qsZcPhHYkPByxdfaLPHXw"]},"events":[{"id":{"txDigest":"CuGC3z2x6SFhgRrkkEcN6AmrhaXNfmxgpEan7xVx1Kru","eventSeq":"0"},"packageId":"0x6cd5f2d1fc2be1a60d5a0aee8bb7dd24802203d6d0d6223f79c74eb2e77702f2","transactionModule":"recast","sender":"0x786dff8a4ee13d45b502c8f22f398e3517e6ec78aa4ae564c348acb07fad7f50","type":"0x6cd5f2d1fc2be1a60d5a0aee8bb7dd24802203d6d0d6223f79c74eb2e77702f2::recast::EventHere","parsedJson":{"u32_vals":{"f0":10,"f1":10},"u64_val":"42949672970"},"bcs":"2Ed1ciJkuxeHonwSERoKiX"}],"results":[{"returnValues":[[[10,0,0,0,10,0,0,0],"0x6cd5f2d1fc2be1a60d5a0aee8bb7dd24802203d6d0d6223f79c74eb2e77702f2::recast::U32_VALS"]]},{}]},"id":4}

and the result field is

"results": [
      {
        "returnValues": [
          [
            [
              10,
              0,
              0,
              0,
              10,
              0,
              0,
              0
            ],
            "0x6cd5f2d1fc2be1a60d5a0aee8bb7dd24802203d6d0d6223f79c74eb2e77702f2::recast::U32_VALS"
          ]
        ]
      },
      {}
    ]

In which, I didn't see the intermediate return values. Maybe I did something wrong in the contract here?

[amnn]

Your PTB contains two move calls, one to create a U32_VALS called recast, and one to use it, called try_recast -- only the first of these actually returns a result, and you can see it in returnValues, which contains an a BCS-encoded byte stream representing the two u32s inside U32_VALS and the type of the returned value.

[github-actions[bot]]

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days.

[amnn]

Closing this PR for now, as we still need the features provided by IndexMap.