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

Fix hardware wallet plugin protobuf incompatibility #667

Merged

Conversation

angrybayblade
Copy link

@angrybayblade angrybayblade commented Sep 21, 2023

Proposed changes

Background -> We want to bump protobuf so that we can remove the forked dependencies, but bumping the protobuf to required version crashes the hardware wallet plugin. This PR adds a warning in the HWI plugin so the user is aware about how to fix this issue

Fixes

If it fixes a bug or resolves a feature request, be sure to link to that issue.

Types of changes

What types of changes does your code introduce to agents-aea?
Put an x in the boxes that apply

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

Put an x in the boxes that apply.

  • I have read the CONTRIBUTING doc
  • I am making a pull request against the develop branch (left side). Also you should start your branch off our develop.
  • Lint and unit tests pass locally with my changes and CI passes too
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that code coverage does not decrease.
  • I have added necessary documentation (if appropriate)
  • Any dependent changes have been merged and published in downstream modules

@codecov-commenter
Copy link

codecov-commenter commented Sep 22, 2023

Codecov Report

❗ No coverage uploaded for pull request base (feat/bump-protocol-gen@d7dc2ee). Click here to learn what that means.
Patch has no changes to coverable lines.

Additional details and impacted files
@@                    Coverage Diff                    @@
##             feat/bump-protocol-gen     #667   +/-   ##
=========================================================
  Coverage                          ?   91.99%           
=========================================================
  Files                             ?      371           
  Lines                             ?    29539           
  Branches                          ?        0           
=========================================================
  Hits                              ?    27175           
  Misses                            ?     2364           
  Partials                          ?        0           
Flag Coverage Δ
unittests 91.99% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines +137 to +140
@pytest.mark.skipif(
condition=(platform.system() == "Windows"),
reason="https://github.com/golang/go/issues/51007",
)
Copy link
Author

Choose a reason for hiding this comment

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

I suggest we address this issue on a separate PR

@angrybayblade angrybayblade force-pushed the fix/hwi-plugin-protobuf-incompatibility branch from 328558d to 30a02c5 Compare September 22, 2023 08:57
@angrybayblade angrybayblade merged commit 8a6d466 into feat/bump-protocol-gen Sep 22, 2023
26 of 32 checks passed
@DavidMinarsch DavidMinarsch deleted the fix/hwi-plugin-protobuf-incompatibility branch October 12, 2023 23:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants