Refactor method names and where asset, balance, and import logic lives #41

alex-stone · 2024-05-10T22:05:42Z

What changed? Why?

This renames some methods to be more idiomatic, e.g. address.list_balances => address.balances.

This also moves the responsibility of some logic into the class that best represents the domain of that behavior.

Wallet Import
- user.import_wallet => Coinbase::Wallet.import (the helper still exists, but logic belongs to Wallet).
Balance Map
- Coinbase.to_balance_map => Coinbase::BalanceMap.from_balances
Asset denomination handling
- Logic existed at the wallet and address layer now belongs to Asset.
Coinbase::Balance handles parsing client balances and converting them to the write asset using the asset class methods.
address.list_transfer_ids => address.transfers.
Change ID methods to not stutter
- user.user_id => user.id
- wallet.wallet_id => wallet.id
- address.address_id => address.id

TODO:

Move asset and amount manipulation to the backend. Supporting gwei and wei and taking an argument to indicate if we're passing whole or atomic units will allow us to not have to do any of this logic client side.
Move wallet creation logic out of Wallet.new so we can leverage wallets in a state where we do not need a seed to be imported.
- This enables us to add user.wallets or Coinbase::Wallet.list that returns a set of wallets

Qualified Impact

This contains some breaking changes from v0.0.3.

This migrates the get_balance* calls to balance* to be more inline with standard ruby method naming, i.e. the method name should reflect what is being returned v.s. what the method is doing.

This removes the reference to the unused FaucetLimitReached error and updates docs to refer to the correct FaucetLimitReachedError

This enables you to import a wallet with the export data directly.

This migrates the balance map parsing off of the top-level Coinbase class and onto the Coinbase::BalanceMap class as a class method. This also exposes a `Coinbase::Balance` object for simple parsing of Coinbase::Client::Balances.

alex-stone · 2024-05-10T22:07:04Z

lib/coinbase/address.rb

-      when :eth
-        amount / BigDecimal(Coinbase::WEI_PER_ETHER.to_s)
-      when :gwei
-        amount / BigDecimal(Coinbase::GWEI_PER_ETHER.to_s)
-      when :usdc
-        amount / BigDecimal(Coinbase::ATOMIC_UNITS_PER_USDC.to_s)


This was not handling weth properly here. Moving this logic to Coinbase::Asset will allow us to more easily support more assets until we push this to the backend.

alex-stone · 2024-05-10T22:09:32Z

lib/coinbase/wallet.rb

@@ -84,54 +117,31 @@ def default_address
    # Returns the Address with the given ID.
    # @param address_id [String] The ID of the Address to retrieve
    # @return [Address] The Address
-    def get_address(address_id)
+    def address(address_id)


This one feels a bit weird to me since we're already exposing addresses.

In what flow will someone have an on-chain address string that is associated with a wallet, where they already have the wallet in memory, and do not have a reference to the address object?

I suspect it might be more likely that they have an address without a wallet and want to be able to search for it and return it if the user/project owns the wallet and address?

jazz-cb · 2024-05-10T22:16:10Z

lib/coinbase/balance.rb

+# frozen_string_literal: true
+
+module Coinbase
+  # A representation of an Balance.


of a Balance or of an Asset Balance?

What's the difference in your opinion there? We can clarify it is an amount of a specific asset?

This makes it so that our fetches of the resource ID doesn't stutter with the class name. ```ruby wallet = Wallet.import(...) puts wallet.id ```

This consolidates the transforms around assets and balances to their respective classes, and moves them out of wallet and address. This also adds testing and fixes the handling for fetching balances of `weth` using `get_address_balance` and `get_wallet_balance` which was not handled properly. The `list_address_balances` and `list_wallet_balances` handling properly handled `weth`.

This makes it so that we return address.transfers instead of address.list_transfer_ids.

jazz-cb · 2024-05-10T22:27:33Z

lib/coinbase/address.rb

@@ -35,45 +35,32 @@ def wallet_id

    # Returns the Address ID.
    # @return [String] The Address ID
-    def address_id
+    def id


should we do the same for transfer_id?

jazz-cb · 2024-05-10T22:35:13Z

lib/coinbase/user.rb

@@ -15,7 +15,7 @@ def initialize(model)

    # Returns the User ID.
    # @return [String] the User ID
-    def user_id
+    def id
      @model.id


also update reference in to_s

jazz-cb · 2024-05-10T22:37:30Z

README.md

@@ -163,7 +163,7 @@ The below code demonstrates how to re-instantiate a Wallet from the data export.
 ```ruby
 # The Wallet can be re-instantiated using the exported data.
 # w4 will be equivalent to w3.
-w4 = u.import_wallet(data)
+w4 = Coinbase::Wallet.import(data)


update line 174 to w5 = wallets[w3.id]

Can we make the README / example something that is actually evaluated?

This updates all model's fields and adds specs that test the inspect function to ensure we're evaluating those functions in test.

alex-stone added 5 commits May 10, 2024 14:32

Migrate get_balance(s) to balance(s)

eb36317

This migrates the get_balance* calls to balance* to be more inline with standard ruby method naming, i.e. the method name should reflect what is being returned v.s. what the method is doing.

Migrate list_addresses to addresses

f89912e

[chore] Cleanup faucet error

7cc33ef

This removes the reference to the unused FaucetLimitReached error and updates docs to refer to the correct FaucetLimitReachedError

Add Wallet.import method for importing a wallet

841a4d0

This enables you to import a wallet with the export data directly.

Migrate balance map to class method

4185ff0

This migrates the balance map parsing off of the top-level Coinbase class and onto the Coinbase::BalanceMap class as a class method. This also exposes a `Coinbase::Balance` object for simple parsing of Coinbase::Client::Balances.

alex-stone commented May 10, 2024

View reviewed changes

alex-stone requested review from yuga-cb, John-peterson-coinbase and jazz-cb May 10, 2024 22:10

jazz-cb reviewed May 10, 2024

View reviewed changes

alex-stone added 4 commits May 10, 2024 15:21

Migrate model.model_id to model.id

0f94795

This makes it so that our fetches of the resource ID doesn't stutter with the class name. ```ruby wallet = Wallet.import(...) puts wallet.id ```

Return transfers on address object

8fed7b4

This makes it so that we return address.transfers instead of address.list_transfer_ids.

[chore] Update changelog

7634f72

alex-stone force-pushed the stone/refactor branch from 08cc6a5 to 7634f72 Compare May 10, 2024 22:22

jazz-cb reviewed May 10, 2024

View reviewed changes

Update all ID fields and add specs

46632bd

This updates all model's fields and adds specs that test the inspect function to ensure we're evaluating those functions in test.

alex-stone requested a review from jazz-cb May 13, 2024 13:04

yuga-cb added 2 commits May 13, 2024 09:37

Change transfer_id => id

5e198ce

Fix unit tests

b5eaffc

yuga-cb merged commit 3f8661a into v0.0.4 May 13, 2024
4 checks passed

yuga-cb deleted the stone/refactor branch May 13, 2024 13:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor method names and where asset, balance, and import logic lives #41

Refactor method names and where asset, balance, and import logic lives #41

alex-stone commented May 10, 2024 •

edited

alex-stone May 10, 2024

alex-stone May 10, 2024

jazz-cb May 10, 2024

alex-stone May 13, 2024

jazz-cb May 10, 2024

jazz-cb May 10, 2024

jazz-cb May 10, 2024

alex-stone May 13, 2024

Refactor method names and where asset, balance, and import logic lives #41

Refactor method names and where asset, balance, and import logic lives #41

Conversation

alex-stone commented May 10, 2024 • edited

What changed? Why?

Qualified Impact

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex-stone commented May 10, 2024 •

edited