Skip to content

Read keys from file#110

Open
BuyHigh-SellLow wants to merge 2 commits intorhettre:mainfrom
BuyHigh-SellLow:read_keys_from_file
Open

Read keys from file#110
BuyHigh-SellLow wants to merge 2 commits intorhettre:mainfrom
BuyHigh-SellLow:read_keys_from_file

Conversation

@BuyHigh-SellLow
Copy link

No description provided.

@rhettre
Copy link
Owner

rhettre commented Dec 29, 2024

What is spot_limit_buy.py? Do we need it?

@BuyHigh-SellLow
Copy link
Author

closes #36, i think

@BuyHigh-SellLow
Copy link
Author

removed spot_limit_buy.py

@rhettre
Copy link
Owner

rhettre commented Jan 4, 2025

Thank you for this contribution! This is a valuable addition that improves security and usability. I particularly appreciate:

  1. The security-first approach of keeping credentials in a separate JSON file
  2. The flexibility to support multiple credential files for different portfolios
  3. The clear documentation in the README
  4. The .gitignore updates to prevent credential leaks

Important note: This PR adds a new secure way to handle credentials without removing the existing ability to use hardcoded API keys. While we recommend using the new secure method for production, having both options allows for rapid prototyping and different use cases.

I'd like to merge this with a few enhancements to make it even more robust:

  1. Add error handling to load_api_credentials():
def load_api_credentials(json_file_path: str = 'cdp_api_key.json') -> Tuple[str, str]:
    """
    Load Coinbase API credentials from a JSON file.

    Args:
        json_file_path (str): Path to the JSON file containing credentials.
            Defaults to 'cdp_api_key.json'.

    Returns:
        Tuple[str, str]: A tuple containing (api_key, api_secret).

    Raises:
        FileNotFoundError: If the credentials file doesn't exist.
        KeyError: If the required fields are missing from the JSON file.
        json.JSONDecodeError: If the file contains invalid JSON.
    """
    if not os.path.exists(json_file_path):
        raise FileNotFoundError(f"Credentials file not found: {json_file_path}")

    try:
        with open(json_file_path, 'r') as file:
            data = json.load(file)
        
        if 'name' not in data or 'privateKey' not in data:
            raise KeyError("JSON file must contain 'name' and 'privateKey' fields")
        
        return data['name'], data['privateKey']
    
    except json.JSONDecodeError:
        raise json.JSONDecodeError(f"Invalid JSON in credentials file: {json_file_path}")
  1. Replace use_include.py with a more secure example that doesn't print credentials:
from include import load_api_credentials
from coinbase_advanced_trader.enhanced_rest_client import EnhancedRESTClient

def get_client(credentials_file: str = 'cdp_api_key.json') -> EnhancedRESTClient:
    """Create a client instance using credentials from a file."""
    api_key, api_secret = load_api_credentials(credentials_file)
    return EnhancedRESTClient(api_key=api_key, api_secret=api_secret)

if __name__ == "__main__":
    try:
        client = get_client()
        accounts = client.get_accounts()
        print("Successfully connected to Coinbase Advanced Trade API")
    except Exception as e:
        print(f"Error connecting to API: {e}")
  1. Consider moving the example to an examples/ directory for better organization

  2. Update the README to clearly show both credential options:

# Option 1: Direct API key usage (good for prototyping)
api_key = "organizations/{org_id}/apiKeys/{key_id}"
api_secret = "-----BEGIN EC PRIVATE KEY-----\n...\n-----END EC PRIVATE KEY-----\n"
client = EnhancedRESTClient(api_key=api_key, api_secret=api_secret)

# Option 2: Secure credential file (recommended for production)
from include import load_api_credentials
api_key, api_secret = load_api_credentials('cdp_api_key.json')
client = EnhancedRESTClient(api_key=api_key, api_secret=api_secret)

Would you be willing to make these changes? Once updated, I'll be happy to merge this PR.

Also, would you be interested in adding some tests for the credential loading functionality? I can provide guidance on that if needed.

Thanks again for your contribution to improving the library's security and usability!

@BuyHigh-SellLow
Copy link
Author

BuyHigh-SellLow commented Jan 7, 2025

I can work on those changes with the exception of the test. I think the credential loading test should be a part of larger CI/CD logic which is ran as part of linting or validation before merging into the code. A separate ticket for that?

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