Skip to content
This repository has been archived by the owner on Jan 13, 2025. It is now read-only.

cli: solana-tokens, validate inputs gracefully #33926

Conversation

norwnd
Copy link
Contributor

@norwnd norwnd commented Oct 30, 2023

Problem

Resolves #16489.

Summary of Changes

Hey @CriesofCarrots, I took this task on to get a bit more comfortable with Rust, lets see if it fits what you had in mind.

@mergify mergify bot added community Community contribution need:merge-assist labels Oct 30, 2023
@mergify mergify bot requested a review from a team October 30, 2023 13:37
tokens/src/commands.rs Outdated Show resolved Hide resolved
tokens/src/commands.rs Outdated Show resolved Hide resolved
tokens/src/commands.rs Outdated Show resolved Hide resolved
tokens/src/commands.rs Outdated Show resolved Hide resolved
tokens/src/commands.rs Outdated Show resolved Hide resolved
@norwnd norwnd requested a review from CriesofCarrots October 31, 2023 08:35
Copy link
Contributor

@CriesofCarrots CriesofCarrots left a comment

Choose a reason for hiding this comment

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

Awesome, the upfront parsing is a great improvement! Thanks for implementing that.
I've added a few suggestions. You'll also want to run cargo clippy to catch a little lint. I'll run CI on this after your next push.

tokens/src/commands.rs Outdated Show resolved Hide resolved
tokens/src/commands.rs Show resolved Hide resolved
tokens/src/commands.rs Show resolved Hide resolved
@norwnd norwnd force-pushed the cli-solana-tokens-validate-inputs-gracefully branch from 30dfbaa to b9c5a07 Compare November 1, 2023 14:46
@norwnd
Copy link
Contributor Author

norwnd commented Nov 1, 2023

Fixed couple other "typos" too, thanks for reviewing @CriesofCarrots ! Should be ready for another round.

Copy link
Contributor

@CriesofCarrots CriesofCarrots left a comment

Choose a reason for hiding this comment

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

This is looking really close! A couple small suggestions, and then please rebase on #33937 and update those helpers in spl_token.rs.

tokens/src/commands.rs Outdated Show resolved Hide resolved
tokens/src/commands.rs Outdated Show resolved Hide resolved
tokens/src/commands.rs Outdated Show resolved Hide resolved
@norwnd norwnd force-pushed the cli-solana-tokens-validate-inputs-gracefully branch from b9c5a07 to 67cc4cd Compare November 1, 2023 18:59
@norwnd norwnd force-pushed the cli-solana-tokens-validate-inputs-gracefully branch from 67cc4cd to a77649e Compare November 1, 2023 19:08
Copy link
Contributor

@CriesofCarrots CriesofCarrots left a comment

Choose a reason for hiding this comment

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

This code looks great! A couple last nits from me in the unit test.
I'll also kick off CI now; feel free to hold off more pushes until we see if that turns up anything I've missed.

tokens/src/commands.rs Outdated Show resolved Hide resolved
tokens/src/commands.rs Outdated Show resolved Hide resolved
@CriesofCarrots CriesofCarrots added the CI Pull Request is ready to enter CI label Nov 1, 2023
@solana-grimes solana-grimes removed the CI Pull Request is ready to enter CI label Nov 1, 2023
@CriesofCarrots
Copy link
Contributor

Ah, cargo fmt

@norwnd
Copy link
Contributor Author

norwnd commented Nov 2, 2023

Ah, cargo fmt

Apologies for this one, I need some time to get used to Rust tooling. Fixed, hopefully.

@CriesofCarrots CriesofCarrots added the CI Pull Request is ready to enter CI label Nov 2, 2023
@solana-grimes solana-grimes removed the CI Pull Request is ready to enter CI label Nov 2, 2023
Copy link

codecov bot commented Nov 2, 2023

Codecov Report

Merging #33926 (aef79bb) into master (25a29c9) will increase coverage by 0.0%.
Report is 1 commits behind head on master.
The diff coverage is 90.6%.

@@           Coverage Diff            @@
##           master   #33926    +/-   ##
========================================
  Coverage    81.9%    81.9%            
========================================
  Files         809      809            
  Lines      218250   218396   +146     
========================================
+ Hits       178793   178946   +153     
+ Misses      39457    39450     -7     

Copy link
Contributor

@CriesofCarrots CriesofCarrots left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks for the contribution, and for all the polish!

@CriesofCarrots CriesofCarrots merged commit 1c00d5d into solana-labs:master Nov 3, 2023
19 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
community Community contribution need:merge-assist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

solana-tokens panics on bad recipient
3 participants