1
\$\begingroup\$

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
    Float(self.account['native_balance']['amount']) ## TODO: Use the buy/quote endpoint instead
  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
\$\endgroup\$
2
\$\begingroup\$

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.
\$\endgroup\$
1
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – 0112 Mar 24 '18 at 22:06
  • \$\begingroup\$ Sure, but the phrase “one of the purposes” is telling, there. A class would ideally have one purpose. \$\endgroup\$ – Rorshark Mar 26 '18 at 16:52
  • \$\begingroup\$ 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". \$\endgroup\$ – Rorshark Mar 26 '18 at 16:56
  • \$\begingroup\$ 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. \$\endgroup\$ – 0112 Apr 18 '18 at 3:16
1
\$\begingroup\$
  • 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]

  attr_reader :symbol, :api_client

  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
    account['native_balance']['amount'] # TODO: Use the buy/quote endpoint instead
  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.

\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – 0112 Mar 25 '18 at 2:30
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ – 0112 Apr 18 '18 at 3:10

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.