Skip to content

Conversation

@thechinedu
Copy link
Contributor

@thechinedu thechinedu commented Apr 23, 2018

What does this PR do?

It adds in backend support for coinbase oauth workflow on the mobile app

Note: There are some test env variables that need to be set in order for the oauth flow to work and this is available upon request.

npm-debug.log Outdated
@@ -0,0 +1,48 @@
0 info it worked if it ends with ok
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you should probably not add this file to the repo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch

return cb(err)
}

redisClient.hmset('coinbase-credentials', {
Copy link
Contributor

@samparsky samparsky Apr 30, 2018

Choose a reason for hiding this comment

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

Using 'coinbase-credentials' has key for hmset shouldn't be, as for every user the previous credentials would be overwritten.

According to redis docs

Sets the specified fields to their respective values in the hash stored at key. This command overwrites any specified fields already existing in the hash. If key does not exist, a new key holding a hash is created.

I think you should use the temp_code as key since its unique to each user.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that makes a lot of sense

return cb(err)
}

return redisClient.hgetall('coinbase-credentials', function (err, obj) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you should use the temp_code as key to retreive user credentials

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants