-
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 372 preparesettlement should calculate needed atas create atas #240
Sdk 372 preparesettlement should calculate needed atas create atas #240
Conversation
SDK-372 prepareSettlement should calculate needed ATAs, create ATAs, and handle required all txs
For example, should also mint options |
|
convergence, | ||
optionMarket.optionMint, | ||
caller | ||
); | ||
|
||
const writerToken = await getOrCreateATA( | ||
if (optionToken.instruction) { |
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.
Avoid deeply nested control flow statements.
convergence, | ||
optionMarket!.writerTokenMint, | ||
caller | ||
); | ||
|
||
const underlyingToken = await getOrCreateATA( | ||
if (writerToken.instruction) { |
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.
Avoid deeply nested control flow statements.
convergence, | ||
optionMarket!.underlyingAssetMint, | ||
caller | ||
); | ||
|
||
if (underlyingToken.instruction) { |
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.
Avoid deeply nested control flow statements.
@@ -166,28 +184,41 @@ export const mintEuropeanOptions = async ( | |||
? stableMintToken | |||
: underlyingMintToken; | |||
|
|||
const optionDestination = await getOrCreateATA( | |||
const { |
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 (optionDestinationAtaIx) { | ||
instructions.push(optionDestinationAtaIx); | ||
} | ||
const { |
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.
…e-rfq/convergence-sdk into sdk-372-preparesettlement-should-calculate-needed-atas-create-atas
convergence, | ||
optionMarket.optionMint, | ||
caller | ||
); | ||
|
||
const writerToken = await getOrCreateATA( | ||
if (optionToken.txBuilder) { |
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.
Avoid deeply nested control flow statements.
convergence, | ||
optionMarket!.writerTokenMint, | ||
caller | ||
); | ||
|
||
const underlyingToken = await getOrCreateATA( | ||
if (writerToken.txBuilder) { |
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.
Avoid deeply nested control flow statements.
convergence, | ||
optionMarket!.underlyingAssetMint, | ||
caller | ||
); | ||
|
||
if (underlyingToken.txBuilder) { |
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.
Avoid deeply nested control flow statements.
}); | ||
}); | ||
let signedTxs: Transaction[] = []; | ||
if (ataTxBuilderArray.length > 0 || mintTxBuilderArray.length > 0) { |
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.
Identical blocks of code found in 2 locations. Consider refactoring.
} | ||
} | ||
let signedTxs: Transaction[] = []; | ||
if (ataTxBuilderArray.length > 0 || mintTxBuilderArray.length > 0) { |
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.
Identical blocks of code found in 2 locations. Consider refactoring.
|
||
const sig = await txBuilder.sendAndConfirm(convergence, confirmOptions); | ||
return sig; | ||
if (ataSignedTx.length > 0) { |
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 4 locations. Consider refactoring.
.serializeAndSendTransaction(signedTx, lastValidBlockHeight) | ||
); | ||
} | ||
if (mintSignedTx.length > 0) { |
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 4 locations. Consider refactoring.
const ataSignedTx = signedTxs.slice(0, ataTxBuilderArray.length); | ||
const mintSignedTx = signedTxs.slice(ataTxBuilderArray.length); | ||
|
||
if (ataSignedTx.length > 0) { |
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 4 locations. Consider refactoring.
.serializeAndSendTransaction(signedTx, lastValidBlockHeight) | ||
); | ||
} | ||
if (mintSignedTx.length > 0) { |
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 4 locations. Consider refactoring.
} | ||
} | ||
if (mintInstructions.length > 0) { |
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 (mintInstructions.length > 0) { |
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 sig = await txBuilder.sendAndConfirm(convergence, confirmOptions); | ||
return sig; | ||
if (ataSignedTx.length > 0) { |
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.
) | ||
); | ||
} | ||
if (mintSignedTx.length > 0) { |
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.
const ataSignedTx = signedTxs.slice(0, ataTxBuilderArray.length); | ||
const mintSignedTx = signedTxs.slice(ataTxBuilderArray.length); | ||
|
||
if (ataSignedTx.length > 0) { |
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.
); | ||
}; | ||
|
||
const doesRfqLegContainsPsyoptionsEuropean = (rfq: Rfq) => { |
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.
@@ -117,150 +99,140 @@ export const createEuropeanProgram = async (convergence: Convergence) => { | |||
); | |||
}; | |||
|
|||
export const mintEuropeanOptions = async ( | |||
// create European Option ATAs and mint options | |||
export const prepareEuropeanOptions = 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 prepareEuropeanOptions
has 121 lines of code (exceeds 25 allowed). Consider refactoring.
@@ -308,3 +357,15 @@ export const prepareSettlementBuilder = async ( | |||
} | |||
); | |||
}; | |||
|
|||
const doesRfqLegContainsPsyoptionsAmerican = (rfq: Rfq) => { |
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.
// used in UI | ||
export const getOrCreateAmericanOptionATAs = async ( | ||
//create American Options ATAs and mint Options | ||
export const prepareAmericanOptions = 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 prepareAmericanOptions
has 113 lines of code (exceeds 25 allowed). Consider refactoring.
) | ||
); | ||
} | ||
if (mintSignedTx.length > 0) { |
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 4 locations. Consider refactoring.
const ataSignedTx = signedTxs.slice(0, ataTxBuilderArray.length); | ||
const mintSignedTx = signedTxs.slice(ataTxBuilderArray.length); | ||
|
||
if (ataSignedTx.length > 0) { |
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 4 locations. Consider refactoring.
return ATAExistence.EXISTS; | ||
let signedTxs: Transaction[] = []; | ||
const lastValidBlockHeight = await convergence.rpc().getLatestBlockhash(); | ||
if (ataTxBuilderArray.length > 0 || mintTxBuilderArray.length > 0) { |
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.
Identical blocks of code found in 2 locations. Consider refactoring.
// used in UI | ||
export const getOrCreateAmericanOptionATAs = async ( | ||
//create American Options ATAs and mint Options | ||
export const prepareAmericanOptions = 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 prepareAmericanOptions
has a Cognitive Complexity of 21 (exceeds 5 allowed). Consider refactoring.
new PublicKey(psyoptionsEuropean.programId) | ||
); | ||
}; | ||
|
||
export const mintEuropeanOptions = async ( | ||
// create European Option ATAs and mint options | ||
export const prepareEuropeanOptions = 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 prepareEuropeanOptions
has a Cognitive Complexity of 20 (exceeds 5 allowed). Consider refactoring.
|
||
const optionMarket = await psyoptionsAmerican.getOptionByKey( | ||
const optionMarket = await leg.getOptionMeta(); |
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 do we skip all the logic if this returns null?
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.
resolved
|
||
const optionMarket = await psyoptionsAmerican.getOptionByKey( | ||
const optionMarket = await leg.getOptionMeta(); | ||
if (optionMarket) { |
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.
We can reverse this logic to remove the amount of nesting
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.
resolved
packages/js/src/utils/classes.ts
Outdated
); | ||
}; | ||
private matchInstruction = (ixToBeAdded: TransactionInstruction): boolean => { | ||
this.IxArray.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.
Why not just return every
as in the method above?
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.
bcos we are checking if one of the ix in IxArray match the passed ix and return true in that case
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.
You can still use it. Just check if every instruction differs using every
and return false in that case. Otherwise, return true.
packages/js/src/utils/classes.ts
Outdated
checked++; | ||
} | ||
}); | ||
return checked === ixLength; |
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.
This wouldn't work correctly in a case when this returns false, but still adds some of the instruction to the inner tracking list
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.
resolved
} | ||
const euroMeta = await leg.getOptionMeta(); | ||
const { stableMint } = euroMeta; | ||
const { underlyingMint } = euroMeta; |
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.
Two deconstructions of the same object
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.
resolved
new PublicKey(psyoptionsEuropean.programId) | ||
); | ||
}; | ||
|
||
export const mintEuropeanOptions = async ( | ||
// create European Option ATAs and mint options | ||
export const prepareEuropeanOptions = 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 prepareEuropeanOptions
has 120 lines of code (exceeds 25 allowed). Consider refactoring.
// used in UI | ||
export const getOrCreateAmericanOptionATAs = async ( | ||
//create American Options ATAs and mint Options | ||
export const prepareAmericanOptions = 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 prepareAmericanOptions
has 108 lines of code (exceeds 25 allowed). Consider refactoring.
// used in UI | ||
export const getOrCreateAmericanOptionATAs = async ( | ||
//create American Options ATAs and mint Options | ||
export const prepareAmericanOptions = 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 prepareAmericanOptions
has a Cognitive Complexity of 18 (exceeds 5 allowed). Consider refactoring.
}); | ||
return false; | ||
}; | ||
checkedAdd(ix: TransactionInstruction | TransactionBuilder): boolean { |
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 checkedAdd
has a Cognitive Complexity of 6 (exceeds 5 allowed). Consider refactoring.
} | ||
} | ||
const optionMarket = await leg.getOptionMeta(); | ||
if (!optionMarket) { |
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.
In which cases this is null?
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.
removing this check
packages/js/src/utils/classes.ts
Outdated
); | ||
}; | ||
private matchInstruction = (ixToBeAdded: TransactionInstruction): boolean => { | ||
this.IxArray.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.
You can still use it. Just check if every instruction differs using every
and return false in that case. Otherwise, return true.
packages/js/src/utils/classes.ts
Outdated
const instructions = ix.getInstructions(); | ||
const ixLength = instructions.length; | ||
let checked = 0; | ||
instructions.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.
Why not use every
here also?
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.
resolved
tx.recentBlockhash = latestBlockHash.blockhash; | ||
tx.feePayer = convergence.rpc().getDefaultFeePayer().publicKey; | ||
await convergence.identity().signTransaction(tx); | ||
await convergence.rpc().serializeAndSendTransaction(tx, latestBlockHash); |
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.
Can't we use a regular sign and send tx
rpc call here?
const signedTx = await convergence.identity().signTransaction(createTx); | ||
await convergence | ||
.rpc() | ||
.serializeAndSendTransaction(signedTx, latestBlockHash); |
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.
Can't we use a regular sign and send tx
rpc call here?
// used in UI | ||
export const getOrCreateAmericanOptionATAs = async ( | ||
//create American Options ATAs and mint Options | ||
export const prepareAmericanOptions = 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 prepareAmericanOptions
has 105 lines of code (exceeds 25 allowed). Consider refactoring.
// used in UI | ||
export const getOrCreateAmericanOptionATAs = async ( | ||
//create American Options ATAs and mint Options | ||
export const prepareAmericanOptions = 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 prepareAmericanOptions
has a Cognitive Complexity of 16 (exceeds 5 allowed). Consider refactoring.
packages/js/src/utils/classes.ts
Outdated
if (ix instanceof TransactionBuilder) { | ||
const instructions = ix.getInstructions(); | ||
const ixLength = instructions.length; | ||
let checked = 0; |
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 do we need this variable? We can just use the return value of instructions.every
in the next line
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.
resolved
packages/js/src/utils/classes.ts
Outdated
const instructions = ix.getInstructions(); | ||
const uniqueIxs = instructions.every((ix) => { | ||
return !this.matchInstruction(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.
Let's rework lambda from a block to a simple expression and rename a uniqueIxs
because this name implies a list of unique instructions, but it's actually a boolean value
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.
resolved
Code Climate has analyzed commit b655a68 and detected 18 issues on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
Fix :
Move MintoOptions logic to prepareSettlement Operation and implement signAllTransaction