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: Enhance contract state management and CLI listing #78

Conversation

JohnPhamous
Copy link
Contributor

  • Add new utils.ts for contract state logic
  • Refactor contract types to support more precise status handling
  • Implement getContractState and getContractStateColor utilities
  • Add --all flag to contract list command for comprehensive contract display
  • Improve contract filtering and state determination

- Add new `utils.ts` for contract state logic
- Refactor contract types to support more precise status handling
- Implement `getContractState` and `getContractStateColor` utilities
- Add `--all` flag to contract list command for comprehensive contract display
- Improve contract filtering and state determination
Copy link

semanticdiff-com bot commented Feb 23, 2025

Review changes with  SemanticDiff

Changed Files
File Status
  src/lib/buy/index.tsx  100% smaller
  src/lib/contracts/ContractDisplay.tsx  18% smaller
  src/lib/contracts/index.tsx  16% smaller
  src/lib/contracts/types.ts  5% smaller
  package.json  0% smaller
  src/lib/contracts/utils.ts  0% smaller

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This PR enhances contract management by introducing state-based filtering and improving code organization through dedicated utility functions.

  • Added new src/lib/contracts/utils.ts with getContractState and getContractStateColor functions for centralized state management
  • Introduced --all flag in src/lib/contracts/index.tsx to optionally display expired contracts in listing
  • Refactored src/lib/contracts/types.ts with clearer contract type definitions, though status field handling needs improvement
  • Modified src/lib/contracts/ContractDisplay.tsx to use new utility functions for consistent state determination
  • Improved error message formatting in src/lib/buy/index.tsx for cancelled orders

6 file(s) reviewed, 4 comment(s)
Edit PR Review Bot Settings | Greptile

Comment on lines 76 to 77
const activeContract = contract as ActiveContract;
const state = getContractState(activeContract);
Copy link

Choose a reason for hiding this comment

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

logic: Type assertion could fail if contract.status is not 'active'. Consider adding a runtime check for contract.status before casting.

Suggested change
const activeContract = contract as ActiveContract;
const state = getContractState(activeContract);
if (contract.status !== 'active') {
continue;
}
const activeContract = contract as ActiveContract;
const state = getContractState(activeContract);

Comment on lines +13 to +14
export interface ActiveContract extends BaseContract {
status: ContractStatus;
Copy link

Choose a reason for hiding this comment

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

logic: ActiveContract's status field should exclude "pending" since that's handled by PendingContract. Change to: status: Exclude<ContractStatus, "pending">

Suggested change
export interface ActiveContract extends BaseContract {
status: ContractStatus;
export interface ActiveContract extends BaseContract {
status: Exclude<ContractStatus, "pending">;

Comment on lines 7 to 10
const startsAt = new Date(contract.shape.intervals[0]);
const endsAt = new Date(
contract.shape.intervals[contract.shape.intervals.length - 1],
);
Copy link

Choose a reason for hiding this comment

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

logic: Missing validation for empty intervals array. Will throw if contract.shape.intervals is empty.

Suggested change
const startsAt = new Date(contract.shape.intervals[0]);
const endsAt = new Date(
contract.shape.intervals[contract.shape.intervals.length - 1],
);
if (!contract.shape.intervals.length) {
throw new Error('Contract must have at least one interval');
}
const startsAt = new Date(contract.shape.intervals[0]);
const endsAt = new Date(
contract.shape.intervals[contract.shape.intervals.length - 1],
);

Comment on lines +22 to +33
export function getContractStateColor(
state: ContractState,
): "green" | "gray" | "cyan" {
switch (state) {
case "Upcoming":
return "green";
case "Expired":
return "gray";
case "Active":
return "cyan";
}
}
Copy link

Choose a reason for hiding this comment

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

style: Missing exhaustive type checking. TypeScript won't catch if a new ContractState is added without updating this function.

Suggested change
export function getContractStateColor(
state: ContractState,
): "green" | "gray" | "cyan" {
switch (state) {
case "Upcoming":
return "green";
case "Expired":
return "gray";
case "Active":
return "cyan";
}
}
export function getContractStateColor(
state: ContractState,
): "green" | "gray" | "cyan" {
switch (state) {
case "Upcoming":
return "green";
case "Expired":
return "gray";
case "Active":
return "cyan";
default:
const exhaustiveCheck: never = state;
throw new Error(`Unhandled contract state: ${exhaustiveCheck}`);
}
}

- Update `getContractState` to accept shape directly instead of full contract
- Remove unnecessary type imports and simplify function signatures
- Adjust contract state calculation to work with more generic shape input
- Improve type consistency across contract-related utilities
@JohnPhamous JohnPhamous merged commit 10c3d96 into main Feb 23, 2025
1 check failed
@JohnPhamous JohnPhamous deleted the johnphamous/eng-1384-do-not-show-expired-contracts-by-default-in-cli branch February 23, 2025 19:19
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.

1 participant