This repository has been archived by the owner. It is now read-only.

Updated library to match new schema format #5

Merged
merged 17 commits into from May 9, 2017

Conversation

Projects
None yet
3 participants
@spankratov
Copy link
Contributor

spankratov commented Apr 30, 2017

No description provided.

@shea256

This comment has been minimized.

Copy link
Contributor

shea256 commented Apr 30, 2017

Awesome thanks for the PR @spankratov! I'll take a look at this tomorrow.

@shea256

This comment has been minimized.

Copy link
Contributor

shea256 commented May 8, 2017

@spankratov Apologies for the delay. @larrysalibra is on this and will be getting to it in the next couple of days.

@larrysalibra

This comment has been minimized.

Copy link
Contributor

larrysalibra commented May 9, 2017

@spankratov This is really great!

I tried it with requests and responses generated by blockstack.js and they work!

Couple of things:

  1. The name look up URL you're using is a temporary one used by the explorer that will be removed. Since we're trying to encourage people and services to run their own Blockstack nodes, I made the blockstack-ruby library default to the node running on localhost and configurable to another host. In blockstack.js, we have the user pass in the name lookup URL instead of providing a default, It would be cool if we can do something like that in the python library as well. If you think it makes more sense to hardcode a default publicly accessible name look up url, https://core.blockstack.org/v2/users/{name} is a better option.

  2. We added an additional step where the app sending the authentication request hosts a manifest file with information about the app. As part of processing the authentication request, the identity provider downloads the manifest from the app. You can see the code here: https://github.com/blockstack/blockstack.js/blob/master/src/auth/authBrowser.js#L84

This is a huge improvement to the blockstack auth library for python. I'm going to merge this PR. Let me know what you think about the above issues. Very happy to accept PRs for them as well. Thanks again!

@larrysalibra larrysalibra merged commit 24f1707 into blockstack-packages:master May 9, 2017

@larrysalibra

This comment has been minimized.

Copy link
Contributor

larrysalibra commented May 9, 2017

@spankratov I created two issues #7 & #8 to track the two items I mentioned above.

@spankratov

This comment has been minimized.

Copy link
Contributor

spankratov commented May 10, 2017

Thanks, @larrysalibra and @shea256 ! Glad my changes were merged. I agree with two issues you mentioned and I'll take a look on it this week.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.