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

fix: output the correct tx info type in swagger #2128

Merged
merged 1 commit into from
Nov 15, 2024

Conversation

compojoom
Copy link
Contributor

I've been trying to use https://redux-toolkit.js.org/rtk-query/usage/code-generation RTK's code-generation with the output of our openAPI. It worked fine, but the generated code was making typescript complain. For example we have this here:

export type Transaction = {
  id: string
  txHash?: string | null
  timestamp?: number | null
  txStatus: string
  txInfo:
    | CreationTransactionInfo
    | CustomTransactionInfo
...
  executionInfo?: (MultisigExecutionInfo | ModuleExecutionInfo) | null
  safeAppInfo?: SafeAppInfo | null
}

export type CreationTransactionInfo = {
  type: string
  humanDescription?: string | null
  creator: AddressInfo
  transactionHash: string
  implementation?: AddressInfo | null
  factory?: AddressInfo | null
  saltNonce?: string | null
}
export type CustomTransactionInfo = {
  type: string
  humanDescription?: string | null
  to: AddressInfo
  dataSize: string
  value?: string | null
  isCancellation: boolean
  methodName?: string | null
  actionCount?: number | null
}

const getTxTo = ({ txInfo }: Pick<Transaction, 'txInfo'>): AddressEx | undefined => {
  switch (txInfo.type) {
    case TransactionInfoType.CREATION: {
      return txInfo.factory
    }
    case TransactionInfoType.TRANSFER: {
      return txInfo.recipient
    }
    case TransactionInfoType.SETTINGS_CHANGE: {
      return undefined
    }
    case TransactionInfoType.CUSTOM: {
      return txInfo.to
    }
  }
}

My Switch statement was hitting the correct case(Creation), but TS was not recognising the txInfo being a CreationTransactionInfo and was complaining that factory doesn't exist. This turned out to stem from the fact that our openAPI is setting type: string and because of this TS was not able to infer the correct type....

Swagger is very picky. Our transactionInfoType is an enum and in every TransactionInfo class we actually know the type of transactionInfo we are dealing with. It’s impossible to have CreationTransactionInfo with a “custom” type. The type is going to be “creation”. By overriding the type with the correct type and setting an ApiProperty with a single value the swagger docs manage to generate a type that is closer to what the TransactionInfo is.
grafik

Swagger is very picky. Our transactionInfoType is an enum and in every
TransactionInfo class we actually know the type of transactionInfo we
are dealing with. It’s impossible to have CreationTransactionInfo with
a “custom” type. The type is going to be “creation”.
By overriding the type with the correct type and setting an ApiProperty
with a single value the swagger docs manage to generate a type that
is closer to what the TransactionInfo is.
@compojoom compojoom requested a review from a team as a code owner November 14, 2024 16:56
Copy link
Member

@hectorgomezv hectorgomezv left a comment

Choose a reason for hiding this comment

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

Thanks! 🚀

@hectorgomezv hectorgomezv merged commit f90e62d into main Nov 15, 2024
20 checks passed
@hectorgomezv hectorgomezv deleted the fix-transaction-info-types branch November 15, 2024 11:27
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.

2 participants