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

WASM32 - Redesigned crate architecture #1497

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

Conversation

chrisgitiota
Copy link

@chrisgitiota chrisgitiota commented Jan 7, 2025

Description of change

This PR introduces the new crate structure as been described in issue #1441.

Links to any relevant issues

Closes #1441

Type of change

  • Bug fix (a non-breaking change which fixes an issue)
  • Enhancement (a non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Fix
  • Non-breaking architectural change

How the change has been tested

  • Current E2E tests for non wasm32 targets run successfully
  • Successfull wasm32 build for identity_iota_core:
    cargo build --package identity_iota_core --lib --target wasm32-unknown-unknown --no-default-features --features iota-client --features revocation-bitmap

Regarding changes in MoveCalls originating from other currently opened PRs

The folder identity_iota_core/src/rebased/iota/move_calls is not used anymore but has not been deleted so far. Changes to this folder, resulting from other currently opened PRs will automatically be merged without problems. After these changes have been merged into identity_iota_core/src/rebased/iota/move_calls they need to be integrated manually into the files identity_iota_core/src/iota_interaction_rust/asset_move_calls.rs and identity_iota_core/src/iota_interaction_rust/identity_move_calls.rs.

After this PR and all other PRs impacting identity_iota_core/src/rebased/iota/move_calls have been merged as described above, the folder identity_iota_core/src/rebased/iota/move_calls must be deleted.

chrisgitiota and others added 18 commits December 4, 2024 12:00
* MoveType has been moved from identity_iota_core to identity_iota_interaction
* The identity_iota_interaction README has been updated to use the latest draft for folder and crate names
…lpha

# Conflicts:
#	identity_iota_core/Cargo.toml
#	identity_iota_core/src/rebased/iota/well_known_networks.rs
…ta_core for the redesigned crate structure

* Compiling identity_iota_core for wasm32 will result in one compiler error
  due to missing wasm-bindings for ProgrammableTransaction
  (TODO in identity_iota_core/src/rebased/transaction.rs)
* identity_iota_core and examples are compilable for non wasm32 platforms
  * Remaining compiler warnings should be investigated
* identity_iota_core e2e tests are compilable but will fail due to serialization/deserialization issues.
  For example:
    IotaTransactionBlockResponse bcs deserialization failed: NotSupported("deserialize_any")
    thread 'identity::adding_controller_works' panicked at /home/christof/Develop/iotaledger/identity.rs/identity_iota_core/src/rebased/transaction.rs:84:14:
    IotaTransactionBlockResponse bcs deserialization failed: NotSupported("deserialize_any")
    stack backtrace:
      0: rust_begin_unwind
      1: core::panicking::panic_fmt
      2: core::result::unwrap_failed
      3: <identity_iota_core::rebased::transaction::TransactionOutput<T> as core::convert::From<identity_iota_core::rebased::transaction::TransactionOutputInternal<T>>>::from
      4: <T as identity_iota_core::rebased::transaction::Transaction>::execute_with_opt_gas::{{closure}}
      5: identity_iota_core::rebased::transaction::Transaction::execute::{{closure}}
      6: e2e::identity::adding_controller_works::{{closure}}
      7: tokio::runtime::scheduler::current_thread::CurrentThread::block_on
      8: tokio::runtime::runtime::Runtime::block_on
      9: e2e::identity::adding_controller_works
      10: core::ops::function::FnOnce::call_once
    note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
…ialization

BCS deserialization is not available for IotaTransactionBlockResponse because the BCS format is not self describing and several struct fields of IotaTransactionBlockResponse are optionally skipped during serialization. JSON deserialization will probably be available but using a cloned instance to convert a TransactionOutputInternal into a TransactionOutput will have a much better efficiency.
…Credential

All tests running successfully for the first time now
@chrisgitiota chrisgitiota added Wasm Related to Wasm bindings. Becomes part of the Wasm changelog No changelog Excludes PR from becoming part of any changelog Rust Related to the core Rust code. Becomes part of the Rust changelog. labels Jan 7, 2025
@chrisgitiota chrisgitiota self-assigned this Jan 7, 2025
@chrisgitiota chrisgitiota requested a review from a team as a code owner January 7, 2025 15:42
* Fix clippy error writing `&Vec` instead of `&[_]` involves a new object where a slice will do
* allow error clippy::too_many_arguments
Copy link
Contributor

@UMR1352 UMR1352 left a comment

Choose a reason for hiding this comment

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

Ooof this is huge. Good job! Fix formatting 👍

@chrisgitiota
Copy link
Author

I'm currently investigating the formating issues.
IMO we need to leave the content of identity_iota_interaction/src/sdk_types as it is (meaning not to rustfmt it) to facilitate upstream merges (issue #1446).

Using the nightly version of rustfmt we could use the unstable option ignore for this. As we are using the +nightly CLI option (according to the console log) this should work.

Otherwise I will give rustfmt::skip a try.

Comment on lines +1 to +2
{(\/\/ Copyright.*\n)*?}// {(Modifications )?}Copyright {(\(c\) )?}{20\d{2}(-20\d{2})?} IOTA Stiftung{(, Fondazione Links)?}{(, Filancore GmbH)?}
{(\/\/ Copyright.*\n)*}// SPDX-License-Identifier: Apache-2.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this update to the template make sense? ^^

Suggested change
{(\/\/ Copyright.*\n)*?}// {(Modifications )?}Copyright {(\(c\) )?}{20\d{2}(-20\d{2})?} IOTA Stiftung{(, Fondazione Links)?}{(, Filancore GmbH)?}
{(\/\/ Copyright.*\n)*}// SPDX-License-Identifier: Apache-2.0
{(\/\/ Copyright.*\n)*?}// {(Modifications )?}Copyright {(\(c\) )?}{20\d{2}(-20\d{2})?} IOTA Stiftung{(?:, .+)?}
// SPDX-License-Identifier: Apache-2.0

This would:

  • keep the list of non-IOTA contributors open, so that we don't have to extend it for contributions from currently unlisted contributors
  • remove the // Copyright prefix from the second line, as I didn't find any cases where we needed it ^^;;

Copy link
Contributor

Choose a reason for hiding this comment

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

Would running cargo +nightly fmt make sense before merging?

Had these changes after running it:

git diff --shortstat
 49 files changed, 423 insertions(+), 506 deletions(-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
No changelog Excludes PR from becoming part of any 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
Development

Successfully merging this pull request may close these issues.

3 participants