-
Notifications
You must be signed in to change notification settings - Fork 7
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: Swap js integration #320
base: main
Are you sure you want to change the base?
Conversation
serve_dir="sdk/python", | ||
resource_deps=["svm-initialize-programs", "auction-server"], | ||
) | ||
|
||
local_resource( | ||
"svm-searcher-js", | ||
serve_cmd="pnpm run testing-searcher-limo --endpoint-express-relay http://127.0.0.1:9000 --chain-id development-solana --private-key-json-file ../../keypairs/searcher_py.json --endpoint-svm http://127.0.0.1:8899 --bid 10000000 --fill-rate 4 --bid-margin 100 --with-latency", | ||
serve_cmd="pnpm run testing-searcher-limo --endpoint-express-relay http://127.0.0.1:9000 --chain-id development-solana --private-key-json-file ../../keypairs/searcher_js.json --endpoint-svm http://127.0.0.1:8899 --bid 1000 --fill-rate 4 --bid-margin 100", |
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.
mint more tokens to everyone and revert bid number to the higher 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.
any reason you are setting lower bid number here? if the bid isn't high enough, you may not be paying for the rent of the fee receiver if it doesn't already have the funds for rent exemption. i think it's fine here bc we airdrop, but worth checking and figuring out the possible scenarios
@@ -25,14 +25,14 @@ repos: | |||
language: "rust" | |||
entry: cargo +nightly-2024-12-03 fmt --manifest-path ./Cargo.toml --all -- --config-path rustfmt.toml | |||
pass_filenames: false | |||
types_or: ["rust", "cargo", "cargo-lock"] |
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.
fix local pre-commit
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.
I had the same issue. Maybe if you upgrade the pre-commit to 4.0.1
or later, it works.
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.
same here, this issue popped up for me out of the blue yesterday. updating pre commit resolved it
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.
yes you should run this hook when Cargo.toml
or Cargo.lock
get updated
}, | ||
solana: { | ||
expressRelayProgram: new PublicKey( | ||
"PytERJFhAKuNNuaiXkApLfWzwNwSNDACpigT3LwQfou" | ||
), | ||
walletRouter: new PublicKey("3hv8L8UeBbyM3M25dF3h2C5p8yA4FptD7FFZu4Z1jCMn"), |
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.
add todo to change later
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.
merge main and get rid of
const expressRelayMetadata = getExpressRelayMetadataPda(chainId); | ||
const svmConstants = SVM_CONSTANTS[chainId]; | ||
|
||
const tokenProgramInput = TOKEN_PROGRAM_ID; |
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.
get from server
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.
I reviewed the server parts. You may need to merge with main. There is a bunch of other changes merged recently.
@@ -25,14 +25,14 @@ repos: | |||
language: "rust" | |||
entry: cargo +nightly-2024-12-03 fmt --manifest-path ./Cargo.toml --all -- --config-path rustfmt.toml | |||
pass_filenames: false | |||
types_or: ["rust", "cargo", "cargo-lock"] |
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.
I had the same issue. Maybe if you upgrade the pre-commit to 4.0.1
or later, it works.
SwapKamino, | ||
#[serde(rename = "limo")] | ||
#[strum(serialize = "limo")] |
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.
strum and serde are doing diff stuff. Are you sure it's safe to remove the strum?
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.
I think strum here will affect the Display
which is used in the tracing.
So after this change it might be with a capital letter in the tracing?
e | ||
)) | ||
})?; | ||
let expected_router_fee_receiver_ta = Pubkey::find_program_address( |
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 should use this address for creating the permission key
// )) | ||
// }, | ||
// )?, | ||
router: self.config.chain_config.wallet_program_router_account, |
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.
if we use the router_fee_receiver_ta
to create the permission key, we don't need to do these checks. actually the permission key will be invalid if the receiver_ta account does not match with what we expect.
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 is a good point, we should probably include the router in the permission_key
construction in get_quote_permission_key
. that's an easy fix, we can mark it as a TODO (or you can add it here after merging main)
@@ -714,7 +753,13 @@ impl Service<Svm> { | |||
) | |||
} | |||
SubmitType::ByServer => { | |||
self.all_signatures_exists(&message_bytes, accounts, &signatures, &[]) | |||
self.relayer_signer_exists(accounts, &signatures)?; |
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.
Does the relayer exist at this point?
State(store): State<Arc<StoreNew>>, | ||
Json(params): Json<QuoteCreate>, | ||
) -> Result<Json<Quote>, RestError> { | ||
let program = get_program(&auth)?; |
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.
I think it's better to make it permissioned at the beginnig
serve_dir="sdk/python", | ||
resource_deps=["svm-initialize-programs", "auction-server"], | ||
) | ||
|
||
local_resource( | ||
"svm-searcher-js", | ||
serve_cmd="pnpm run testing-searcher-limo --endpoint-express-relay http://127.0.0.1:9000 --chain-id development-solana --private-key-json-file ../../keypairs/searcher_py.json --endpoint-svm http://127.0.0.1:8899 --bid 10000000 --fill-rate 4 --bid-margin 100 --with-latency", | ||
serve_cmd="pnpm run testing-searcher-limo --endpoint-express-relay http://127.0.0.1:9000 --chain-id development-solana --private-key-json-file ../../keypairs/searcher_js.json --endpoint-svm http://127.0.0.1:8899 --bid 1000 --fill-rate 4 --bid-margin 100", |
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.
any reason you are setting lower bid number here? if the bid isn't high enough, you may not be paying for the rent of the fee receiver if it doesn't already have the funds for rent exemption. i think it's fine here bc we airdrop, but worth checking and figuring out the possible scenarios
@@ -616,23 +601,71 @@ impl Service<Svm> { | |||
}, | |||
), | |||
}; | |||
let mint_fee = match swap_data.fee_token { |
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.
i think some of this server stuff may be unnecessary once you pull main? pushed some updates recently
e | ||
)) | ||
})?; | ||
let expected_router_fee_receiver_ta = Pubkey::find_program_address( |
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.
i think the whole point of making the router fee receiver ta generic as opposed to constraining it to be the ATA was so that routers could provide arbitrary token accounts
also the wallet_program_router_account is no longer in the config
// )) | ||
// }, | ||
// )?, | ||
router: self.config.chain_config.wallet_program_router_account, |
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 is a good point, we should probably include the router in the permission_key
construction in get_quote_permission_key
. that's an easy fix, we can mark it as a TODO (or you can add it here after merging main)
}, | ||
solana: { | ||
expressRelayProgram: new PublicKey( | ||
"PytERJFhAKuNNuaiXkApLfWzwNwSNDACpigT3LwQfou" | ||
), | ||
walletRouter: new PublicKey("3hv8L8UeBbyM3M25dF3h2C5p8yA4FptD7FFZu4Z1jCMn"), |
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.
merge main and get rid of
return getAssociatedTokenAddressSync( | ||
tokenMintAddress, | ||
owner, | ||
true, |
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.
makes sense why you defined this, but maybe a comment noting that this allows off-curve owners?
tokenProgram: PublicKey | ||
): [PublicKey, TransactionInstruction] { | ||
const ataAddress = getAssociatedTokenAddress(owner, mint, tokenProgram); | ||
const createUserTokenAccountIx = createAssociatedTokenAccountInstruction( |
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 use this helper?
: bidAmount, | ||
referralFeeBps: new anchor.BN(0), | ||
deadline, | ||
feeToken: { input: {} }, |
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 get this from the server. right now, the server is always returning input
), | ||
tokenProgramOutput, | ||
mintOutput, | ||
routerFeeReceiverTa: getAssociatedTokenAddress( |
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 is not necessarily the ATA. the router account here will be the token account that the referrer wants the SPL fee token to be sent to, so you can just use that
@@ -0,0 +1,95 @@ | |||
import argparse |
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.
i think it would be easier (and better) to start writing the tilt scripts in JS? i think you could probably write this whole script more flexibly (using SDK methods rather than writing endpoints and payloads manually) in JS
@@ -133,6 +136,53 @@ export class SimpleSearcherLimo { | |||
return bid; | |||
} | |||
|
|||
/** |
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.
Update this comment
? swapOpportunity.tokens.outputToken.token | ||
: swapOpportunity.tokens.outputToken; | ||
const trader = swapOpportunity.userWalletAddress; | ||
const mintFee = mintInput; |
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.
it should be either mintInput or mintOutput
swapOpportunity.tokens.type === "output_specified" | ||
? new anchor.BN(swapOpportunity.tokens.outputToken.amount) | ||
: bidAmount, | ||
referralFeeBps: new anchor.BN(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.
This should come from server + we need to add it in the permission key on the server side
: bidAmount, | ||
referralFeeBps: new anchor.BN(0), | ||
deadline, | ||
feeToken: { input: {} }, |
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.
it should be constructed based on the swap token type
expressRelayMetadata, | ||
searcher, | ||
trader: swapOpportunity.userWalletAddress, | ||
searcherInputTa: getAssociatedTokenAddress( |
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.
Maybe it's better to make it optional variable?
If not exist set to getAssociatedTokenAddress
otherwise, use the input variable
mintInput, | ||
tokenProgramInput | ||
), | ||
searcherOutputTa: getAssociatedTokenAddress( |
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.
same
), | ||
tokenProgramOutput, | ||
mintOutput, | ||
routerFeeReceiverTa: getAssociatedTokenAddress( |
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.
Maybe it's better to make it Ata inside the program?
No description provided.