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

feat: add Send to internal error #990

Merged
merged 1 commit into from
Nov 28, 2024
Merged

feat: add Send to internal error #990

merged 1 commit into from
Nov 28, 2024

Conversation

tomyrd
Copy link
Collaborator

@tomyrd tomyrd commented Nov 27, 2024

When changing the base branches to next in the Miden Client (0xPolygonMiden/miden-client#617), one of the errors is caused because an element of AssetError is not Send. By changing this line the error in the client is fixed (tested it locally).

@tomyrd tomyrd added the no changelog This PR does not require an entry in the `CHANGELOG.md` file label Nov 27, 2024
Copy link
Contributor

@bobbinth bobbinth 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! Thank you!

On a quick glance I don't see issues with this, but also wonder what @PhilippGackstatter thinks.

Copy link
Contributor

@PhilippGackstatter PhilippGackstatter 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!

I think all the boxed dyn Errors should be Send + Sync + 'static but I forgot that on this one. I'll add it to the error issue: #972.

@tomyrd
Copy link
Collaborator Author

tomyrd commented Nov 28, 2024

Forgot to set up commit signing. I just signed the same commit, I'll wait for the CI just to be sure before merging

@tomyrd tomyrd merged commit 582263a into next Nov 28, 2024
9 checks passed
@tomyrd tomyrd deleted the tomyrd-error-send branch November 28, 2024 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changelog This PR does not require an entry in the `CHANGELOG.md` file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants