-
Notifications
You must be signed in to change notification settings - Fork 133
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
Replace custom hyper client with reqwest #593
Replace custom hyper client with reqwest #593
Conversation
How to build and test: git clone https://github.com/quentinlesceller/grin-wallet.git
cd grin-wallet
git checkout node_client_master
cargo build --release Now try basic stuff such as API, sending transactions etc... |
tACK - ran the full test suite + |
are some of these covered by https://github.com/mimblewimble/grin-wallet/blob/master/tests/owner_v3_lifecycle.rs#L47 ? |
@pkariz could you build this branch and check a few of the wallet API endpoint + making some transaction? 🙏 |
I'm getting:
When trying to receive Grin. |
I think this may be similar to the fix here, but am not sure #552. I do remember seeing a couple of issues e.g. this one where if you click on the linked gist you see the same error message. |
Indeed good catch @phyro ! Will fix in a bit |
Ok I can confirm that it works with the fix in #552. |
Need more testing now. |
@quentinlesceller I'm now wondering how come this change is not present on the master branch. I can never remember the release flow we use but I think we may have forgotten to merge the release branch into master on grin-wallet (or perhaps we don't do this?). If this is the case, I'd remove this commit, sync master with the release branch and then rebase this PR. |
Sorry, I think I know what confused me and I believe it was a false alarm on my end. The fix you added in the 2nd commit is already present on master which is why there is no diff with it on this PR and I didn't check this. We can forget my comment above |
@pkariz could you test the endpoints you used for the exchange on this updated version when you find the time? 🙏 (if you find the time also some random ones) |
Looks like we have a conflict in |
Fixed. |
Tested various operations and interactions and looks good. 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
are we good to merge this one? |
…mimblewimble#593) - Fix for http client
Taken from #520. All credits to @jaspervdm.