-
Notifications
You must be signed in to change notification settings - Fork 0
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
Sdk 331 account management should be moved to instruments #246
Conversation
Nagaprasadvr
commented
Sep 25, 2023
- Added few more methods to LegInstrument Insterface like getBaseAssetMints, getOracleAccounts etc
- cleaned up and removed helper functions associated with rfq legs
…he-options' of https://github.com/convergence-rfq/convergence-sdk into sdk-331-account-management-should-be-moved-to-instruments
|
SDK-331 Account management should be moved to instruments
these clog up handlers. Should add these to instrument. Should not construct accounts directly in operations |
const optionMarketTxBuilder = | ||
TransactionBuilder.make().setFeePayer(payer); | ||
if (optionMarketIxs.length > 0) { | ||
optionMarketIxs.forEach((ix) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar blocks of code found in 2 locations. Consider refactoring.
if (optionMarketIxs.length === 0) continue; | ||
const optionMarketTxBuilder = | ||
TransactionBuilder.make().setFeePayer(payer); | ||
optionMarketIxs.forEach((ix) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar blocks of code found in 2 locations. Consider refactoring.
} | ||
const flattendedTransactions = transactionMatrix.flat(); | ||
const { keypairs, identities } = getSignerHistogram(signers); | ||
for (let transaction of flattendedTransactions) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar blocks of code found in 2 locations. Consider refactoring.
); | ||
return optionMarketIxs; | ||
} | ||
getBaseAssetAccount(): AccountMeta { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar blocks of code found in 3 locations. Consider refactoring.
async getPreparationsBeforeRfqCreation(): Promise<CreateOptionInstrumentsResult> { | ||
return []; | ||
} | ||
getBaseAssetAccount(): AccountMeta { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar blocks of code found in 3 locations. Consider refactoring.
…he-options' of https://github.com/convergence-rfq/convergence-sdk into sdk-331-account-management-should-be-moved-to-instruments
}; | ||
} | ||
|
||
getBaseAssetAccount(): AccountMeta { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar blocks of code found in 3 locations. Consider refactoring.
getDecimals = () => PsyoptionsAmericanInstrument.decimals; | ||
getSide = () => this.side; | ||
toLeg(): Leg { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar blocks of code found in 3 locations. Consider refactoring.
}; | ||
return oracleAccount; | ||
} | ||
toLeg(): Leg { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar blocks of code found in 3 locations. Consider refactoring.
|
||
return optionMarketIxs; | ||
} | ||
toLeg(): Leg { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar blocks of code found in 3 locations. Consider refactoring.
.map((ins) => ins.getValidationAccounts()) | ||
.flat(); | ||
baseAssetAccounts = instrumentsToAdd.map((i) => i.getBaseAssetAccount()); | ||
rfqBuilder = TransactionBuilder.make() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar blocks of code found in 2 locations. Consider refactoring.
|
||
return TransactionBuilder.make() | ||
let rfqBuilder = TransactionBuilder.make() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar blocks of code found in 2 locations. Consider refactoring.
return this.underlyingAssetMint; | ||
} | ||
|
||
async getOracleAccount(): Promise<AccountMeta> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar blocks of code found in 3 locations. Consider refactoring.
return this.mintAddress; | ||
} | ||
|
||
async getOracleAccount(): Promise<AccountMeta> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar blocks of code found in 3 locations. Consider refactoring.
return this.underlyingAssetMint; | ||
} | ||
|
||
async getOracleAccount(): Promise<AccountMeta> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar blocks of code found in 3 locations. Consider refactoring.
@@ -254,3 +368,157 @@ export const createAmericanProgram = ( | |||
|
|||
return americanProgram; | |||
}; | |||
|
|||
export const getPsyAmericanMarketIxs = async ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Function getPsyAmericanMarketIxs
has 44 lines of code (exceeds 25 allowed). Consider refactoring.
@@ -1,4 +1,8 @@ | |||
import { PublicKey, TransactionInstruction } from '@solana/web3.js'; | |||
import { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
File instrument.ts
has 447 lines of code (exceeds 250 allowed). Consider refactoring.
@@ -1,4 +1,9 @@ | |||
import { Keypair, PublicKey, TransactionInstruction } from '@solana/web3.js'; | |||
import { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
File instrument.ts
has 473 lines of code (exceeds 250 allowed). Consider refactoring.
Code Climate has analyzed commit 4383e1c and detected 31 issues on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
…he-options' of https://github.com/convergence-rfq/convergence-sdk into sdk-331-account-management-should-be-moved-to-instruments
instrumentDecimals: legInstrument.getDecimals(), | ||
side: toSolitaLegSide(legInstrument.getSide()), | ||
}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why to move this logic to the instrument interface? The logic in each instrument is the same and is a clone of this logic
getValidationAccounts(): AccountMeta[]; | ||
getPreparationsBeforeRfqCreation(): Promise<CreateOptionInstrumentsResult>; | ||
getUnderlyingBaseAssetMint(): PublicKey; | ||
toLeg(): Leg; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A lot of those method are option-specific, but not generic. For example, they can be confusing when applied to spot instrument, rename getUnderlyingBaseAssetMint