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

Add another option to provide auth details #25

Merged
merged 1 commit into from
May 9, 2024

Conversation

drohit-cb
Copy link
Contributor

Description Of Change

Add an option to initialize client with the api key name and private key instead of completely relying on api-key file.

This PR will enable these 2 styles of interacting with the API and make option 1 as the default in our examples and README.

Option 1: Requires client initialize to pass the details such as API key name and private key.

const apiKeyName: string = 'your-api-key-name';
const apiPrivateKey: string = 'your-api-private-key';

const client = new StakingClient(apiKeyName, apiPrivateKey);

client.Ethereum.stake(network, stakerAddress, amount);

Option 2: Need a api key of name .coinbase_cloud_api_key.json present in the root dir.

const client = new StakingClient();

client.Ethereum.stake(network, stakerAddress, amount);

Testing Procedure

Manually ran through the README to execute and make sure all is good.

@@ -175,13 +186,17 @@ This code sample returns rewards for an Ethereum validator address. View the ful
// examples/ethereum/list-rewards.ts
import { StakingClient } from "@coinbase/staking-client-library-ts";

// Set your api key name and private key here. Get your keys from here: https://portal.cdp.coinbase.com/access/api
const apiKeyName: string = 'your-api-key-name';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

what would be really cool and nice to have is a "test" API key that perhaps has limited scopes etc that I can plaster in all examples. Could be easy for someone wanting to just run the example as is, without going to the platform.

…ey instead of completely relying on api-key file
@drohit-cb drohit-cb force-pushed the add_option_to_init_client_from_api_key_info branch from d69e145 to e21e1d0 Compare May 8, 2024 21:20
@drohit-cb drohit-cb marked this pull request as ready for review May 8, 2024 21:20
npm install -g ts-node typescript
```

3. Get your API keys info such as api key name and api private key from here: https://portal.cdp.coinbase.com/access/api. <br>
Copy link
Contributor

Choose a reason for hiding this comment

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

I really like how the examples don't assume the user placed the API Key at a specific location anymore. It's best to be explicit in the code where possible. However, I do think it's a bit more cumbersome for a user to manually copy/paste the key's two fields into the example (including a private key, which isn't ideal from a security perspective). Especially since the CDP GUI downloads the key onto the filesystem already.

What if we asked the user to specify the file location of the API Key? So maybe it looks like this?

const apiKeyLocation: string = './.coinbase_cloud_api_key.json';

const client = new StakingClient(apiKeyLocation);

This is just an idea, so curious what you think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It depends what we are trying to do here - something I probably failed calling out in the PR description.. you're right downloading an API key or copy/pasting these 2 values feel equally difficult - it felt like exchanging one problem with perhaps a bigger one 😅 . But the primary aim of moving to coy/pasting these 2 details - key name and private key is so that:

  1. We can move away from the coinbase_cloud api key file naming that the platform might take some time to update. We can skip the name confusion if we just copy/paste the relevant fields which the platform offers a good way of copy/pasting.

  2. I believe the platform team is working on a token based auth model at which point in time, copy/pasting a token from platform UI will be the expected path. So this PR in a way preps for that eventual future. @deangalvin-cb can maybe confirm/deny this or add more color.

Choose a reason for hiding this comment

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

👍 Yes, we are working directly with the https://github.com/coinbase/coinbase-sdk-ruby team to see if we can better align & unify our designs. I had @drohit-cb go this route as to not spend too much time building each iteration, and just making this quick improvement now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Kk I'm onboard 👍

Comment on lines +31 to +35
3. Get your API keys info such as api key name and api private key from here: https://portal.cdp.coinbase.com/access/api. <br>
These will be used in order to set up our client later in the example code. <br>
For detailed instructions refer to our api key setup guide [here](https://docs.cdp.coinbase.com/developer-platform/docs/cdp-keys).

4. Copy and paste one of the code samples below or any of our [provided examples](./examples/) into an `example.ts` file and run it with `ts-node` :rocket:
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 we can use fewer words here 🤔.

Suggested change
3. Get your API keys info such as api key name and api private key from here: https://portal.cdp.coinbase.com/access/api. <br>
These will be used in order to set up our client later in the example code. <br>
For detailed instructions refer to our api key setup guide [here](https://docs.cdp.coinbase.com/developer-platform/docs/cdp-keys).
4. Copy and paste one of the code samples below or any of our [provided examples](./examples/) into an `example.ts` file and run it with `ts-node` :rocket:
3. Download an API Key from the [portal](https://portal.cdp.coinbase.com/access/api). For more information about API Keys, refer to [our docs](https://docs.cdp.coinbase.com/developer-platform/docs/cdp-keys).
4. Copy one of the code samples below into an `example.ts` file and run: :rocket:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tl;dr want to move away from downloading the file completely

@drohit-cb drohit-cb requested a review from ProfMoo May 9, 2024 15:53
@drohit-cb drohit-cb merged commit 29ff1a0 into main May 9, 2024
3 checks passed
@drohit-cb drohit-cb deleted the add_option_to_init_client_from_api_key_info branch May 9, 2024 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants