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

refactor(governance-sdk): DRYer Borsh classes, remove repeated properties & constructors #496

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

johnrees
Copy link
Contributor

@johnrees johnrees commented Jan 21, 2022

This is a proposal to refactor serializable Borsh classes so that there is no repetition of their arguments, as this can create a lot of extra lines of code that devs might potentially forget to update when adding/removing/updating struct properties.

Example 1. Class with no custom properties and/or instance methods

before

export class ChatMessageBody {
  type: ChatMessageBodyType;
  value: string;

  constructor(args: { type: ChatMessageBodyType; value: string }) {
    this.type = args.type;
    this.value = args.value;
  }
}

// dev needs to specify/assign `type` and `value` multiple times

after

export interface ChatMessageBody {
  // only need to declare `type` and `value` in one place
  type: ChatMessageBodyType;
  value: string;
}
// ChatMessageBody is empty because constructor initialization is handled by BorshClass
export class ChatMessageBody extends BorshClass<ChatMessageBody> {}

Example 2. Class with custom properties and/or instance methods

before

export class ChatMessage {
  accountType = GovernanceChatAccountType.ChatMessage;

  proposal: PublicKey;
  author: PublicKey;
  postedAt: BN;
  replyTo: PublicKey | undefined;
  body: ChatMessageBody;

  constructor(args: {
    proposal: PublicKey;
    author: PublicKey;
    postedAt: BN;
    replyTo: PublicKey | undefined;
    body: ChatMessageBody;
  }) {
    this.proposal = args.proposal;
    this.author = args.author;
    this.postedAt = args.postedAt;
    this.replyTo = args.replyTo;
    this.body = args.body;
  }
}

after

// intermediary interface declared as extra property `accountType` is not to be passed to the constructor
interface ChatMessageProps {
  proposal: PublicKey;
  author: PublicKey;
  postedAt: BN;
  replyTo?: PublicKey;  // ? makes `| undefined` union type redundant
  body: ChatMessageBody;
}
// export interface as in example 1, effectively a union of ChatMessageProps above, and ChatMessage below
export interface ChatMessage extends ChatMessageProps {}
export class ChatMessage extends BorshClass<ChatMessageProps> {
  // add properties and methods as usual, which will not to be passed in as args to the constructor
  accountType = GovernanceChatAccountType.ChatMessage;
}

TODO

  • check if this is problematic with boolean arguments that are set to false when they are undefined i.e. this.isWritable = !!args.isWritable;
  • double check nothing has been broken, maybe add some new tests

@vercel
Copy link

vercel bot commented Jan 21, 2022

@johnrees is attempting to deploy a commit to the Solana Team on Vercel.

A member of the Team first needs to authorize it.

@vercel
Copy link

vercel bot commented Jan 21, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/solana-labs/oyster-governance/9HgRZw78xnb7Y5XVCwf4u4FbRiGD
✅ Preview: https://oyster-governance-git-fork-johnrees-dry-solana-labs.vercel.app

Comment on lines +106 to +110
export abstract class BorshClass<T> {
constructor(args: T) {
Object.assign(this, args);
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

BorshClass is just a simple helper class that assigns the keys from the args object to properties on the instance

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this could probably be a decorator tbh but I didn't want to get too weird with it yet

@johnrees johnrees changed the title refactor(governance-ui): DRYer Borsh classes, remove repeated properties & constructors refactor(governance-sdk): DRYer Borsh classes, remove repeated properties & constructors Jan 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant