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

Update iota.rs to include timeout bugfix #712

Merged
merged 3 commits into from
Mar 15, 2022

Conversation

PhilippGackstatter
Copy link
Contributor

Description of change

Update iota.rs to the latest dev commit in order to include the timeout bugfix from iotaledger/iota.rs#848. Previously, the requet_timeout builder option was essentially ignored, and since this is the only option we expose, the fix is important to us.

This also comments out the currently unused identity-comm crate, because libjose's rsa = 0.5 dependency requires zeroize < 1.5, which is incompatible with the iota.rs requirement of (now) 1.5.3 of said crate. identity-comm is currently unused and will need a refactor anyway.

Type of change

Add an x to the boxes that are relevant to your changes.

  • 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

How the change has been tested

Ran a local test (not added to the code base) to make sure the fix works for us:

#[tokio::test]
async fn test_timeout() {
  let client = ClientBuilder::new()
    .request_timeout(std::time::Duration::from_millis(1))
    .build()
    .await
    .unwrap();

  let error = client.publish_json("index", &[0u8; 32]).await.unwrap_err();

  assert!(matches!(error, crate::Error::ClientError(iota_client::Error::ReqwestError(_))));
}

Change checklist

Add an x to the boxes that are relevant to your changes.

  • I have followed the contribution guidelines for this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@PhilippGackstatter PhilippGackstatter added Bug Something isn't working. Rust Related to the core Rust code. Becomes part of the Rust changelog. labels Mar 14, 2022
@PhilippGackstatter PhilippGackstatter self-assigned this Mar 14, 2022
@PhilippGackstatter PhilippGackstatter added the Patch Change without affecting the API that requires a patch release. Part of "Patch" section in changelog label Mar 14, 2022
@cycraig
Copy link
Contributor

cycraig commented Mar 14, 2022

Broken in Wasm due to timing code added in iotaledger/iota.rs@47e5f66

@eike-hass
Copy link
Collaborator

Broken in Wasm due to timing code added in iotaledger/iota.rs@47e5f66

How did you catch this / how could we have caught that in CI?

@cycraig
Copy link
Contributor

cycraig commented Mar 15, 2022

It was caught in CI https://github.com/iotaledger/identity.rs/runs/5541464592?check_suite_focus=true

@eike-hass
Copy link
Collaborator

It was caught in CI https://github.com/iotaledger/identity.rs/runs/5541464592?check_suite_focus=true

that is good to hear

Copy link
Contributor

@cycraig cycraig left a comment

Choose a reason for hiding this comment

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

Looks good to me! Annoying to retain dead code but it will be addressed Soon™.

@PhilippGackstatter PhilippGackstatter merged commit 7131407 into dev Mar 15, 2022
@PhilippGackstatter PhilippGackstatter deleted the chore/update-iota-client branch March 15, 2022 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working. Patch Change without affecting the API that requires a patch release. Part of "Patch" section in changelog Rust Related to the core Rust code. Becomes part of the Rust changelog.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants