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

create rust cli tooling #123

Merged
merged 58 commits into from
Jul 26, 2023
Merged

create rust cli tooling #123

merged 58 commits into from
Jul 26, 2023

Conversation

Johnnycus
Copy link
Contributor

closes #122


if let Err(e) = client.get_account(&mint_lp_tokens_to) {
if !mint_lp_tokens_to.eq(&from_ata) {
panic!(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

is this how i throw error?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah so as in stakedex-cli I decided that since it's just a simple cli program, panicking is fine and probably what we want to do when we encounter an error so that the user can just get feedback on what went wrong and the program exits.

Also you probably learnt this from the book alrdy but just in case: panicking is also what happens when .unwrap() fails

Note that in larger/long-running applications (like the stakedex-api server), you probably never want to panic since that means your entire application crashes

Copy link
Member

Choose a reason for hiding this comment

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

I think for learning purposes, you can look into defining explicit error type

let msg = Message::new(&instructions, Some(&payer.pubkey()));
let blockhash = client.get_latest_blockhash().unwrap();
let mut tx = Transaction::new(&signers.iter().collect(), msg, blockhash);
let sig = client.send_and_confirm_transaction(&tx).unwrap();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

is this how i create, sign and send tx?

Copy link
Contributor

Choose a reason for hiding this comment

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

yep, when in doubt refer to stakedex-cli: https://github.com/igneous-labs/stakedex-cli/blob/master/src/subcmd/fund_sol_bridge.rs , https://github.com/igneous-labs/stakedex-cli/blob/master/src/tx_utils.rs

We might want to add a --dry-run flag for this too but that can come later

lamports_to_sol(total_fees_lamports)
);
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this file has a lot of errors, not sure how to fix them

if err.is_some() {
return;
}
// if inner_instructions.is_some() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

how can i check if this is falsey

.collect();

for (sig_idx, c) in confirmed_txs.iter().enumerate() {
// if c.is_none() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

how can i check if this is falsey

Copy link
Contributor

@billythedummy billythedummy left a comment

Choose a reason for hiding this comment

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

I think we can also just add a #[derive(Debug)] on top of ProtocolFee Fee and Pool in programs/unstake so that they impl debug and then change all the println statements in fetch_protocol_fee and view_pool to stuff like println!("Fee:\n{:#?}", fee) instead of referencing all the individual fields

Also theres probably some more cargo clippy warnings to resolve

cli-rust/src/subcmd/add_liquidity.rs Outdated Show resolved Hide resolved
cli-rust/src/subcmd/add_liquidity.rs Outdated Show resolved Hide resolved
cli-rust/src/subcmd/add_liquidity.rs Outdated Show resolved Hide resolved
cli-rust/src/subcmd/fetch_protocol_fee.rs Outdated Show resolved Hide resolved
cli-rust/src/subcmd/fetch_protocol_fee.rs Show resolved Hide resolved
Johnnycus and others added 29 commits July 25, 2023 10:06
@Johnnycus Johnnycus merged commit 8438427 into v1-remove-payer Jul 26, 2023
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.

3 participants