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

feat: Adding Address Class with tests #10

Merged
merged 7 commits into from
May 15, 2024

Conversation

erdimaden
Copy link
Contributor

What changed? Why?

  • Adding Base Address Class with tests
  • faucet_transaction_test.ts file name updated and removing unused import

Qualified Impact

import { AddressClient } from "./types";

/**
* Class representing an Address in the Coinbase SDK.
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets be consistent on information in class docs across SDK's

Reference Ruby SDK - https://github.com/coinbase/coinbase-sdk-ruby/blob/eb7326c69a67bd8d4000d3169c01e3adfd40e97d/lib/coinbase/address.rb#L11

private client: AddressClient;

/**
* Creates an instance of Address.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Creates an instance of Address.
* Initializes a new Address instance.

To align with User constructor documentation

}

/**
* Requests faucet funds for the address.
Copy link
Contributor

Choose a reason for hiding this comment

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

note: only supported on testnet networks

);
return new FaucetTransaction(response.data);
} catch (e) {
throw new Error(`Failed to request faucet funds`);
Copy link
Contributor

Choose a reason for hiding this comment

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

We should add the api response code and message to the error thrown.

I think there is a ticket for this? Let's handle this soon so we don't have to retroactively replace the temporary thrown errors in many places.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, I'm going to throw AxiosError for failed HTTP request.

src/coinbase/address.ts Outdated Show resolved Hide resolved
src/coinbase/types.ts Outdated Show resolved Hide resolved
@erdimaden erdimaden force-pushed the feat/address-class-implementation branch from c5da5d2 to f7be1d3 Compare May 14, 2024 20:56
@erdimaden erdimaden force-pushed the feat/address-class-implementation branch from f7be1d3 to 19563da Compare May 14, 2024 20:57
@erdimaden erdimaden force-pushed the feat/address-class-implementation branch from e32db61 to d52fc66 Compare May 14, 2024 21:53
@erdimaden erdimaden force-pushed the feat/address-class-implementation branch from 54b009a to 4cc5c7b Compare May 14, 2024 22:19
Comment on lines 46 to 66
it("should request faucet funds and return a FaucetTransaction", async () => {
axiosMock.onPost().reply(200, {
transaction_hash: "mocked_transaction_hash",
});
const address = new Address(VALID_ADDRESS_MODEL, client);
const faucetTransaction = await address.faucet();
expect(faucetTransaction).toBeInstanceOf(FaucetTransaction);
expect(faucetTransaction.getTransactionHash()).toBe("mocked_transaction_hash");
});

it("should request faucet funds and throw an InternalError if the request does not return a transaction hash", async () => {
axiosMock.onPost().reply(200, {});
const address = new Address(VALID_ADDRESS_MODEL, client);
await expect(address.faucet()).rejects.toThrow("Failed to complete faucet request");
});

it("should throw an AxiosError if faucet request fails", async () => {
axiosMock.onPost().reply(400);
const address = new Address(VALID_ADDRESS_MODEL, client);
await expect(address.faucet()).rejects.toThrow("Request failed with status code 400");
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets group these in a describe for the method. There also is a few things missing for Ruby SDK parity in these tests.

  1. Faucet limit reached failure case
  2. let(:tx_hash) { '0xdeadbeef' } - value for transaction hash response is different

reference

Comment on lines 33 to 35
expect(address.getPublicKey()).toBe(newEthAddress.publicKey);
expect(address.getNetworkId()).toBe(VALID_ADDRESS_MODEL.network_id);
expect(address.getWalletId()).toBe(VALID_ADDRESS_MODEL.wallet_id);
Copy link
Contributor

Choose a reason for hiding this comment

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

Separate describe block tests for the getter methods?

Ruby SDK Reference


it("should request faucet funds and throw an InternalError if the request does not return a transaction hash", async () => {
axiosMock.onPost().reply(200, {});
const address = new Address(VALID_ADDRESS_MODEL, client);
Copy link
Contributor

Choose a reason for hiding this comment

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

these can be moved to beforeEach block

@erdimaden erdimaden force-pushed the feat/address-class-implementation branch from bbef36f to f01cf6a Compare May 15, 2024 20:13
…reateAxiosMock and registerAxiosInterceptors functions - Updating Address and Coinbase class test cases
@erdimaden erdimaden force-pushed the feat/address-class-implementation branch from f01cf6a to 7cd9c7d Compare May 15, 2024 20:14
@erdimaden erdimaden merged commit b34089d into master May 15, 2024
4 checks passed
@erdimaden erdimaden deleted the feat/address-class-implementation branch June 21, 2024 18:04
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.

2 participants