# Calculate cryptocurrency gains and losses using the Coinbase API

This class calculates the gains and losses (in USD) from the four leading cryptocurrencies using the Coinbase API. (The full repo can be found here)

In addition to general feedback there are some specific items I would like feedback on:

• What can I do to improve my tests? (For example, is it silly of me to test only that a method returns a float like in .crypto_amount_in_wallet? If so are there any other ways I could go about testing this method, that wouldn't just be a test of the Coinbase API?)
• Am I using the VCR gem in a way that makes sense?
• Is there a better or more conventional way to mock data than recording the VCR cassettes and modifying the responses by hand?
• Should I consider separating functionality that purely wraps API endpoints into a separate class, or leave them where they are?

### Class

require 'coinbase/wallet'
require 'dotenv'

class Currency
def initialize(symbol:, api_client:)
raise ArgumentError 'Must specify currency symbol (BTC BCH LTC ETH)' if symbol.nil? || !([:BTC, :LTC, :BCH, :LTC].include?(symbol))
raise ArgumentError 'Currency requires a valid coinbase client.' if api_client.nil? || api_client.class != Coinbase::Wallet::Client

@symbol                   = symbol
@api_client               = api_client
@account                  = @api_client.account(symbol)
@crypto_amount_in_wallet  = @account['balance']['amount']
@usd_invested             = @account['native_balance']['amount']
end

def symbol
return @symbol
end

def api_client
return @api_client
end

def account
account = self.api_client.account(self.symbol)
account.refresh!
return account
end

def crypto_amount_in_wallet
return Float(self.account['balance']['amount'])
end

def usd_invested
transactions   = self.account.transactions
total_invested = transactions
.map { |t| t['native_amount']['amount'].to_f }
.reduce(:+)
return Float(total_invested)
end

def current_cash_in_value
end

def usd_lost
loss = self.usd_invested - self.current_cash_in_value
if loss.negative? # i.e. $1.00 -$10.00 = -$9.00 means that$9.00 have been made as profit, so return a $0.00 loss. return 0.0 else return loss end end def usd_gained gain = self.current_cash_in_value - self.usd_invested if gain.negative? # i.e.$1.00 - $100.00 = -$99.00 means that $99.00 have been lost as profit, so return a$0.00 as a gain.
return 0.0
else
return gain
end
end
end


### Spec

require 'rspec'
require_relative '../lib/currency.rb'

describe Currency do
before (:all) do
VCR.use_cassette('client_and_currency') do
@api_client = Coinbase::Wallet::Client.new(api_key:    ENV['COINBASE_KEY'],
api_secret: ENV['COINBASE_SECRET'])
@currency   = Currency.new(symbol: :BTC, api_client: @api_client)
end
end

describe '#initialize' do
it 'raises an ArgumentError when a new currency is instantiated without a symbol' do
expect { Currency.new(api_client: @api_client) }.to raise_error ArgumentError
end

it 'raises an ArgumentError if no coinbase client object is passed' do
expect { Currency.new(symbol: :BTC) }.to raise_error ArgumentError
end

it 'returns a new object of type "Currency"' do
VCR.use_cassette('currency_init') do
expect(Currency.new(symbol: :BTC, api_client: @api_client)).to be_a_kind_of Currency
end
end
end

describe '.symbol' do
it 'returns a symbol' do
expect(@currency.symbol).to be_a Symbol
end

it 'is one of :BTC, :LTC, :BCH, :ETH' do
expect([:BTC, :LTC, :BCH, :ETH].include?(@currency.symbol)).to be true
end
end

describe '.api_client' do
it 'properly instantiates a coinbase client' do
expect(@currency.api_client).to be_a Coinbase::Wallet::Client
end

it 'doesn\'t raise an error' do
expect { @currency.api_client }.not_to raise_error
end
end

describe '.account' do
it 'returns a hash' do
VCR.use_cassette('account_hash') do
expect(@currency.account).to be_a Hash
end
end

it 'has 11 keys' do
VCR.use_cassette('account_hash') do
expect(@currency.account.keys.count).to eql(11)
end
end

it 'matches the symbol' do
VCR.use_cassette('account_hash') do
expect(@currency.symbol.to_s).to eq(@currency.account['currency'])
end
end
end

describe '.crypto_amount_in_wallet' do
it 'is a float' do
VCR.use_cassette('crypto_amount_in_wallet') do
expect(@currency.crypto_amount_in_wallet).to be_a Float
end
end
end

describe '.usd_invested' do
it 'is a float' do
VCR.use_cassette('account') do
expect(@currency.usd_invested).to be_a Float
end
end
end

describe '.current_cash_in_value' do
it 'is a float' do
VCR.use_cassette('current_cash_in_val') do
expect(@currency.current_cash_in_value).to be_a Float
end
end
end

describe '.usd_lost' do
context 'given no loss' do
it 'should return 0.0' do
VCR.use_cassette('usd_no_loss') do
expect(@currency.usd_lost).to eql(0.0)
end
end
end

context 'given a loss' do
it 'should return 9.00 as a loss' do
VCR.use_cassette('usd_loss') do
expect(@currency.usd_lost).to eql(10.0 - 1.0)
end
end
end
end

describe '.usd_gained' do
context 'with no gain' do
it 'returns 0.0 as a gain' do
VCR.use_cassette('usd_no_gain') do
expect(@currency.usd_gained).to eql(0.0)
end
end
end

context 'with a gain' do
it 'returns 10.0 as a gain' do
VCR.use_cassette('usd_gain') do
expect(@currency.usd_gained).to eql(10.0)
end
end
end
end

end


Rorshark's points are important: if your class is doing too many things, it probably means you have other classes hiding in there. Before you ask about how good your tests are, you should be asking if you are testing the right things.

The defined methods in your class:

• symbol
• api_client
• account
• crypto_amount_in_wallet
• usd_invested
• current_cash_in_value
• usd_lost
• usd_gained

The two constructor arguments (symbol, api_client) aren't used except in the account method. This tells me that the class may be abstracted too far, or that you need a separate account class as Rorshark suggested.

You have a currency, but it's not the main thing here. The main thing seems to be the account and the operations on the account (amount_in_wallet, invested, cash_value, usd_lost, usd_gained). The currency value is just an argument needed to get a handle on the account.

So, if you renamed the class 'Account', made the account method private or just turned it into a class variable you'd have something a little more cohesive.

Some stylistic nits:

• def symbol ... and def api_client ... could be condensed to attr_reader :symbol, :api_client

• it's a little more Ruby-esque to skip return for end-of-method return values, but not a critical thing

Some things to be careful with:

• using floats when it comes to currency will come back to bite you. Consider converting everything to integers early (multiply currency by some precision factor: 100, 1000, 10,000, or even 100,000) and then you can just use integer math everywhere until it's time to display.

Since most of the useful methods here involve doing agregate calculations on “account”, you may want to consider renaming the class to “Account” and passing in the account hash in the initializer rather than passing in an api client and currency code.

That would isolate the actual logic from the coinbase api entirely and would make your code much easier to test.

You wouldnt need VCR at all if you did that.

• Not a bad idea, I feel like doing so would invalidate one of the purposes behind the class though: which is to get up to date account data each time the account getter is called. – 0112 Mar 24 '18 at 22:06
• Sure, but the phrase “one of the purposes” is telling, there. A class would ideally have one purpose. – Rorshark Mar 26 '18 at 16:52
• Also, a class already exists for "getting up to date account data", it's called Coinbase::Wallet::Client, which I'm assuming does its own validation of "currency symbol". – Rorshark Mar 26 '18 at 16:56
• The purpose of the class is to provide up to date data about a specific currency. Hence why it's a Currency class not an Account class. Not requesting the data each time (as you've suggested) would invalidate the purpose of the class. Which is to abstract each currency for a specific user into its own object, in as close to real time as possible. This class does have one purpose. – 0112 Apr 18 '18 at 3:16
• What can I do to improve my tests? (For example, is it silly of me to test only that a method returns a float like in .crypto_amount_in_wallet? If so are there any other ways I could go about testing this method, that wouldn't just be a test of the Coinbase API?

Yes, your tests are weak, you should focus in the core logic of your class and assert edge cases. Testing the type of the return isn't enough.

• Am I using the VCR gem in a way that makes sense?

Yes, VCR is a good choise when the third-service don't provide testing mocks (example, FOG gem has built-in testing mocks). But you don't need to test everything with VCR, you can mock some code (to assert edge cases), and use VCR to do integration testing.

• Is there a better or more conventional way to mock data than recording the VCR cassettes and modifying the responses by hand?

I think it's preferable to never modify responses by hand, you should simulate your senario with a real data. The reason is that cassetes should be disposable, if you need to update your testes, or the service API changes, you just need to thrown then way and record new ones.

• Should I consider separating functionality that purely wraps API endpoints into a separate class, or leave them where they are?

@Rorshark justed this, you can create an AccountWrapper to do this.

I applied some improvements in your class applying a little of ruby-style-guide, here is the final result:

(sorry renaming your methods, but you can ignore this :D)

require 'coinbase/wallet'

class AccountStatus
SUPPORTED_SYMBOLS = [:BTC, :LTC, :BCH, :LTC]

def initialize(symbol:, api_client:)
raise ArgumentError 'Must specify currency symbol (BTC BCH LTC ETH)' unless SUPPORTED_SYMBOLS.include?(symbol)
raise ArgumentError 'Currency requires a valid coinbase client.' unless api_client.is_a?(Coinbase::Wallet::Client)

@symbol = symbol
@api_client = api_client
end

def account
@account ||= api_client.account(symbol).tap(&:refresh!)
end

def crypto_balance
account['balance']['amount']
end

def cash_invested
account.transactions.map { |t| t['native_amount']['amount'] }.sum
end

def cash_current
end

def usd_lost
# returns 0.0 on negative values
[cash_invested - cash_current, 0.0].max
end

def usd_gained
# returns 0.0 on negative values
[cash_current - cash_invested, 0.0].max
end
end


Few things to notice:

• I removed some unused variables and .nil? checks on constructor params (nil values will fail the boolean check)
• Cached the @account variable, I think it's not a good idea to keep refreshing it, but you must decide on that
• Removed the conditional on lost/gained methods using Array#max
• Removed Float conversions, I read that the coinbase gem already converts money amounts into BigDecimal ref

I'll work in the test file later and update this answer with it, sorry I'm out of time now.

• Thanks for the time you took for the review and for answering my questions! And I would love feedback on the tests if you have a chance. – 0112 Mar 25 '18 at 2:30
• I would be totally willing to mark this as the answer if you were to provide more feedback on the spec. Specifically if you could point me in the right direction as far as mocking data from the Coinbase::Wallet::Client. – 0112 Apr 18 '18 at 3:10