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

Adding initial version of Coinbase Class #8

Merged
merged 12 commits into from
May 14, 2024
Merged

Conversation

erdimaden
Copy link
Contributor

What changed? Why?

Adding initial version of Coinbase Class. I will have more update in my second PR

@erdimaden erdimaden marked this pull request as ready for review May 13, 2024 22:01
src/coinbase/coinbase.ts Outdated Show resolved Hide resolved
src/coinbase/coinbase.ts Outdated Show resolved Hide resolved
src/coinbase/coinbase.ts Outdated Show resolved Hide resolved
Comment on lines 3 to 10
export class User {
private userId: string = "";
private client: ApiClients;

constructor(userId: string, client: ApiClients) {
this.userId = userId;
this.client = client;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should (User, Wallet, Address etc..) classes create member vars for each of their respective fields or should the class encapsulate the generated model (UserModel) and use the model to retrieve field values in the getters?

The diff would be roughly:

Suggested change
export class User {
private userId: string = "";
private client: ApiClients;
constructor(userId: string, client: ApiClients) {
this.userId = userId;
this.client = client;
}
import { User as UserModel } from "../clients/api");
export class User {
private model: UserModel;
private client: ApiClients;
constructor(model: UserModel, client: ApiClients) {
this.model = model;
this.client = client;
}
public getUserId(): string {
return this.model.user_id:
}

This seems a bit cleaner especially for classes such as Address and Transfer where we would be duplicating / redeclaring lots of fields otherwise and is more similar to what we do with the Ruby SDK.

Curious everyone's thoughts here! @erdimaden @yuga-cb @alex-stone

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While I appreciate the use of a generated model, I personally lean towards including fields within the same class. This approach can help reduce dependency on the generated code and provide more control over modifications that might arise from changes in the generated code

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 not deviate approaches between SDKs. Please use and delegate to the generated model. If we don't think that is the right solution, then we should evaluate that holistically for both the JS and Ruby SDK.

src/coinbase/coinbase.ts Outdated Show resolved Hide resolved
src/coinbase/coinbase.ts Outdated Show resolved Hide resolved
import { ApiClients } from "./types";
import { User } from "./user";

// 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.

When do we use // and when do we use /* style comments? Let's have some consistency around this

Copy link
Contributor Author

@erdimaden erdimaden May 14, 2024

Choose a reason for hiding this comment

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

I added "multiline-comment-style": ["error", "starred-block"], to eslint so with this configuration:

we use // for single-line comments.
we use /* */ for multi-line comments with a starred block style.

https://eslint.org/docs/latest/rules/multiline-comment-style

src/coinbase/coinbase.ts Outdated Show resolved Hide resolved
src/coinbase/coinbase.ts Outdated Show resolved Hide resolved
import { AxiosPromise } from "axios";
import { User as UserModel } from "./../client/api";

export type UserAPIClient = { getCurrentUser(options?): AxiosPromise<UserModel> };
Copy link
Contributor

Choose a reason for hiding this comment

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

JSdoc

src/coinbase/user.ts Outdated Show resolved Hide resolved
src/coinbase/user.ts Outdated Show resolved Hide resolved
@erdimaden erdimaden force-pushed the feature/Coinbase-class branch from b894219 to ea3e0e5 Compare May 14, 2024 02:15
@erdimaden erdimaden force-pushed the feature/Coinbase-class branch from ff342b8 to afb9cf5 Compare May 14, 2024 02:50
@erdimaden erdimaden force-pushed the feature/Coinbase-class branch from afb9cf5 to 15fa64f Compare May 14, 2024 02:53
@erdimaden erdimaden force-pushed the feature/Coinbase-class branch from 00bcb8e to 6d43e54 Compare May 14, 2024 03:52
}
}

export class InvalidConfiguration extends Error {
Copy link
Contributor

Choose a reason for hiding this comment

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

typedoc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have a ticket for Errors but I will add JSDocs for the existing classes
https://jira.coinbase-corp.com/browse/PSDK-115

}
}

export class InternalError extends Error {
Copy link
Contributor

Choose a reason for hiding this comment

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

typedoc

export class InvalidAPIKeyFormat extends Error {
static DEFAULT_MESSAGE = "Invalid API key format";

constructor(message: string = InvalidAPIKeyFormat.DEFAULT_MESSAGE) {
Copy link
Contributor

Choose a reason for hiding this comment

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

typedoc

export const InvalidConfiguration = new Error("Invalid configuration");
export const InvalidAPIKeyFormat = new Error("Invalid format of API private key");
export const InternalError = new Error(`Internal Error`);
export class InvalidAPIKeyFormat extends Error {
Copy link
Contributor

Choose a reason for hiding this comment

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

typedoc

debugging = false,
basePath: string = BASE_PATH,
) {
if (apiKeyName === "" || privateKey === "") {
Copy link
Contributor

Choose a reason for hiding this comment

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

lets explicitly say which one is empty?

export class InternalError extends Error {
static DEFAULT_MESSAGE = "Internal Error";

constructor(message: string = InternalError.DEFAULT_MESSAGE) {
Copy link
Contributor

Choose a reason for hiding this comment

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

typedoc

export class InvalidConfiguration extends Error {
static DEFAULT_MESSAGE = "Invalid configuration";

constructor(message: string = InvalidConfiguration.DEFAULT_MESSAGE) {
Copy link
Contributor

Choose a reason for hiding this comment

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

typedoc

private model: UserModel;
private client: ApiClients;

constructor(user: UserModel, client: ApiClients) {
Copy link
Contributor

Choose a reason for hiding this comment

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

typedoc

this.model = user;
}

public getUserId(): string {
Copy link
Contributor

Choose a reason for hiding this comment

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

typedoc

return this.model.id;
}

toString(): string {
Copy link
Contributor

Choose a reason for hiding this comment

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

typedoc

this.model = user;
}

public getUserId(): string {
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
public getUserId(): string {
public getId(): string {

@erdimaden erdimaden force-pushed the feature/Coinbase-class branch from ec9983d to d6f6b1f Compare May 14, 2024 16:48
@erdimaden erdimaden merged commit 5b50e78 into master May 14, 2024
3 checks passed
@erdimaden erdimaden deleted the feature/Coinbase-class 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.

3 participants