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

CLI remove unwrap_or_default() on rpc calls #33782

Merged
merged 1 commit into from
Oct 20, 2023

Conversation

Arrowana
Copy link
Contributor

@Arrowana Arrowana commented Oct 20, 2023

Problem

When calling some cli commands it might return an incorrect results because the RPC node calls failures are swallowed silently

Running this several times illustrates the problem, some features are disappearing
solana feature status -um Doesn't work well because of the limitation of the public RPC, as a result the printed result is quite unstable

Summary of Changes

Avoid .unwrap_or_default() ? instead to propagate the error

Now the CLI call above gives
Error: RPC response error -32602: Too many inputs provided; max 5

Fixes #

@mergify mergify bot added community Community contribution need:merge-assist labels Oct 20, 2023
@mergify mergify bot requested a review from a team October 20, 2023 10:11
@Arrowana Arrowana changed the title Remove unwrap_or_default() on rpc calls CLI remove unwrap_or_default() on rpc calls Oct 20, 2023
@joncinque joncinque added the CI Pull Request is ready to enter CI label Oct 20, 2023
@solana-grimes solana-grimes removed the CI Pull Request is ready to enter CI label Oct 20, 2023
@codecov
Copy link

codecov bot commented Oct 20, 2023

Codecov Report

Merging #33782 (df1825c) into master (0b05e8d) will increase coverage by 0.0%.
Report is 30 commits behind head on master.
The diff coverage is 0.0%.

@@           Coverage Diff           @@
##           master   #33782   +/-   ##
=======================================
  Coverage    81.8%    81.8%           
=======================================
  Files         806      806           
  Lines      217894   217893    -1     
=======================================
+ Hits       178252   178276   +24     
+ Misses      39642    39617   -25     

Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

This makes sense! Thanks for noticing and fixing

@joncinque joncinque merged commit e1a9f8e into solana-labs:master Oct 20, 2023
31 checks passed
@Arrowana Arrowana deleted the fix/cli-rpc-silently-failing branch October 20, 2023 20:47
@jstarry
Copy link
Member

jstarry commented Nov 13, 2023

Just ran into this, thanks for fixing @Arrowana!

@willhickey willhickey added the v1.17 PRs that should be backported to v1.17 label Nov 13, 2023
mergify bot pushed a commit that referenced this pull request Nov 13, 2023
willhickey pushed a commit that referenced this pull request Nov 14, 2023
… (#34051)

CLI remove unwrap_or_default() on rpc calls (#33782)

(cherry picked from commit e1a9f8e)

Co-authored-by: Pierre <Arrowana@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Community contribution need:merge-assist v1.17 PRs that should be backported to v1.17
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants