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.

Sign up for GitHub

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

Jump to bottom

Prepare public Identity interface for future wasm-bindings #1429

Open
wants to merge 2 commits into
base: feat/identity-rebased
Choose a base branch
from
Open

Prepare public Identity interface for future wasm-bindings #1429

wants to merge 2 commits into from

Conversation

chrisgitiota
Copy link

@chrisgitiota chrisgitiota commented Nov 20, 2024 •
edited
Loading

This PR introduces minor changes to prepare the public Identity interface for future wasm-bindings.

Important questions:

  • All uses of ProgrammableTransaction in our public interface needs to be avoided.
    • identity_iota_core/src/rebased/transaction.rs
      • impl Transaction for ProgrammableTransaction
        Will be replaced with
        impl Transaction for ProgrammableTransactionBcs
        Can this cause any Problems?
    • identity_iota_core/src/rebased/proposals/mod.rs
      • pub trait ProposalT
        fn parse_tx_effects(tx_response: &IotaTransactionBlockResponse)
        IotaTransactionBlockResponse will be replaced with dyn IotaTransactionBlockResponseT object
        Can this cause any Problems?
        If yes see below (IotaTransactionBlockResponse)
  • As the following public usages of rust sdk types can not be avoided anymore:
    Do we need to warn library users in the documentation?
    Are these types/functions only used internal but can not be made pup(crate) ?
    • ProgrammableTransactionBuilder
      • identity_iota_core/src/rebased/proposals/borrow.rs
        • BorrowActionWithIntent where
          F: FnOnce(&mut Ptb, &HashMap<ObjectID, (Argument, IotaObjectData)>)
        • Proposal::with_intent where
          F: FnOnce(&mut Ptb, &HashMap<ObjectID, (Argument, IotaObjectData)>) + Send + 'static
        • ExecuteBorrowTx<'i, BorrowAction>::with_intent where
          F: FnOnce(&mut Ptb, &HashMap<ObjectID, (Argument, IotaObjectData)>)
        • impl<'i, F> Transaction for ExecuteBorrowTx<'i, BorrowActionWithIntent> where
          F: FnOnce(&mut Ptb, &HashMap<ObjectID, (Argument, IotaObjectData)>) + Send
        • pub(crate) IntentFn = Box<dyn FnOnce(&mut Ptb, &HashMap<ObjectID, (Argument, IotaObjectData)>) + Send>
          Ptb needds to be replaced with ProgrammableTransactionBcs
          Effects for library users is unclear
  • IotaTransactionBlockResponse
    This type will be replaced internaly with a dyn IotaTransactionBlockResponseT object. As the library users should not be aware of our internal type, we could use a BCS serealization here. The users would need to de-serialize the IotaTransactionBlockResponseBcs in their code.
    • identity_iota_core/src/rebased/transaction.rs
      • TransactionOutput
        pub response: IotaTransactionBlockResponse

Other Questions:

  • identity_iota_core::rebased::client::IdentityClient
    • Following functions will be moved into the IotaClientAdapter.
      As they are are pub(crate) this will not effect our library users, right?
      • pub(crate) async fn execute_transaction()
      • pub(crate) async fn default_gas_budget()
  • Is this needed anymore?
    • identity_iota_core::rebased::utils - fn get_client()
  • Do our users also implement these traits (could be pub(crate) otherwise?):
    • identity_iota_core::rebased::transaction
      • pub trait Transaction
        • Type 'Output' could be incompatible
        • FYI: Migrated in earlier version in asset::asset.rs
          No changes in Trait definition needed but implementation changed
      • pub trait ProtoTransaction
        • Type 'Input' could be incompatible
    • identity_iota_core::rebased::proposals
      • pub trait ProposalT
        • Types 'Action' and 'Output' could be incompatible
        • FYI: Migrated in earlier version in proposals::config_change.rs,
          • Trait Return value had to be changed:
            dyn TxBuilder object instead of ProgrammableTransactionBuilder
            but this is not needed anymore
    • identity_iota_core::rebased::utils
      • pub trait MoveType
        • Default function try_to_argument() is moved to asset_move_calls.rs
        • FYI: Migrated in earlier version in proposals::config_change.rs,

Informational:

  • Constructor functions for IdentityClientReadOnly:
    There will be target compile switches for IdentityClientReadOnly constructors:
    • identity_iota_core::rebased::client::IdentityClientReadOnly
      • Existing constructors (new() and new_with_network_name()) will be renamed,
        become non public and are called by target switched new constructor functions
      • Target switches for new public constructor functions
  • Our identity_iota_core/src/rebased/sui/move_calls module and all fn definitions are pub(crate).
    There are no "pub fun" in the move_calls folder, so our library users can not use
    them (there are no public re-exports through a pub use directive).

@chrisgitiota chrisgitiota added Wasm Related to Wasm bindings. Becomes part of the Wasm changelog Rust Related to the core Rust code. Becomes part of the Rust changelog. labels Nov 20, 2024
@chrisgitiota chrisgitiota self-assigned this Nov 20, 2024
@chrisgitiota chrisgitiota requested a review from a team as a code owner November 20, 2024 11:58
@chrisgitiota chrisgitiota added the Breaking change A change to the API that requires a major release. Part of "Changed" section in changelog label Nov 20, 2024
@UMR1352
Copy link
Contributor

UMR1352 commented Nov 20, 2024

@chrisgitiota the only problem I see here is with Ptb. Ptb is required to let users craft their own transactions from scratch or to inject some logic into a predefined transaction (see Proposal<BorrowAction> for instance).
I'll try to answer in vague order to your other questions:

  • IdentityClient::execute_transaction should be exposed to users, especially if Transaction trait is not accessible to them.
  • The traits Transaction ProtoTransaction and ProposalT should be exposed to users but I see your problem with them.
  • Everything in the move_calls functions is correctly pub(crate). That module contains code to construct transactions that should only be used internally

@chrisgitiota
Copy link
Author

Regarding the Ptb the users could inject a Ptb instance into our library, like we already do with the IotaClient?

@chrisgitiota
Copy link
Author

IdentityClient::execute_transaction should be exposed to users, especially if Transaction trait is not accessible to them.

Would it be an option to use a BCS serialization of the ProgrammableTransaction for the publicly available IdentityClient::execute_transaction function?

Internally, in our platform agnostic code, we need to use the BCS representation for a ProgrammableTransaction too, as this type is not compilable for WASM32 (hopefully this will change soon).

@UMR1352
Copy link
Contributor

UMR1352 commented Nov 21, 2024 •
edited
Loading

Internally, in our platform agnostic code, we need to use the BCS representation for a ProgrammableTransaction too, as this type is not compilable for WASM32 (hopefully this will change soon).

Sure no problem

@chrisgitiota
Merge branch 'feat/identity-rebased' into feat/interface-changes-to-p…
0d1afbb 
…repare-wasm32-bindings

# Conflicts:
#	identity_iota_core/packages/identity_iota/Move.lock
@chrisgitiota
Copy link
Author

All of the issues addressed in the 'Important questions' section above, should be manageable using adapters converting internally used platform agnostic types (ProgrammableTransactionBcs, dyn IotaTransactionBlockResponseT object, dyn TransactionBuilderT object) into original IOTA Client Rust SDK types (ProgrammableTransaction, IotaTransactionBlockResponse, TransactionBuilder).

Therefore the interface of the types listed in the 'Important questions' section can remain unchanged.

@chrisgitiota
Copy link
Author 

IdentityClient::execute_transaction should be exposed to users, especially if Transaction trait is not accessible to them.

Would it be an option to use a BCS serialization of the ProgrammableTransaction for the publicly available IdentityClient::execute_transaction function?

This can also be handled using an internal adapter converting the ProgrammableTransaction into a platform agnostic ProgrammableTransactionBcs.

The interface of the IdentityClient::execute_transaction() function can remain unchanged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@chrisgitiota chrisgitiota

Labels
Breaking change A change to the API that requires a major release. Part of "Changed" section in changelog Rust Related to the core Rust code. Becomes part of the Rust changelog. Wasm Related to Wasm bindings. Becomes part of the Wasm changelog
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@chrisgitiota @UMR1352