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

Error handling #107

Merged
merged 5 commits into from
Dec 18, 2024
Merged

Error handling #107

merged 5 commits into from
Dec 18, 2024

Conversation

Woody4618
Copy link
Contributor

  • Add priority fee parameter to upload so that it is more reliable
  • Fixed the abort signal so that its possible to stop the --remote command
  • Added as skip build parameter to verify-from-repo so that one does not have to wait until the build is done to upload the on chain data. For example in case the upload fails or there is some other error in between
  • Check if program is actually deployed on the current cluster and show an error if it is not

Copy link
Collaborator

@ngundotra ngundotra left a comment

Choose a reason for hiding this comment

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

Good fixes overall- will help devs troubleshoot when they haven't deployed their own program, or can't land a tx on mainnet url. Added some questions & small nits.

src/api/client.rs Outdated Show resolved Hide resolved
src/api/client.rs Outdated Show resolved Hide resolved
src/api/client.rs Outdated Show resolved Hide resolved
src/api/client.rs Outdated Show resolved Hide resolved
src/api/client.rs Outdated Show resolved Hide resolved
src/api/client.rs Outdated Show resolved Hide resolved
src/api/client.rs Outdated Show resolved Hide resolved
Comment on lines +232 to +235
.arg(Arg::with_name("skip-build")
.long("skip-build")
.help("Skip building and verification, only upload the PDA")
.takes_value(false)))
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is what solana-verify remote submit-job is for

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added this so that you can easily skip the first step of this command and directly go to the PDA upload. Submit job does trigger the remote job not the PDA upload.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay this makes sense.

verify-from-repo --remote will now trigger write to PDA & submit job, skipping build

src/main.rs Show resolved Hide resolved
Comment on lines -901 to -905
if remote {
let genesis_hash = get_genesis_hash(connection)?;
if genesis_hash != MAINNET_GENESIS_HASH {
return Err(anyhow!("Remote verification only works with mainnet. Please omit the --remote flag to verify locally."));
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this entire block getting deleted?

Copy link
Contributor Author

@Woody4618 Woody4618 Dec 17, 2024

Choose a reason for hiding this comment

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

It was duplicate code exactly the same as 100 lines below. Both code paths now use the same args logic. Was added here: a8ea0ce

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you forgot to add this get_genesis_hash check back in

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its still there, no? In line 1140

Copy link
Collaborator

Choose a reason for hiding this comment

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

You're right! Looks good to me

Copy link
Collaborator

@ngundotra ngundotra left a comment

Choose a reason for hiding this comment

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

One last note, all other nits fixed 🫡

Comment on lines +232 to +235
.arg(Arg::with_name("skip-build")
.long("skip-build")
.help("Skip building and verification, only upload the PDA")
.takes_value(false)))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay this makes sense.

verify-from-repo --remote will now trigger write to PDA & submit job, skipping build

Comment on lines -901 to -905
if remote {
let genesis_hash = get_genesis_hash(connection)?;
if genesis_hash != MAINNET_GENESIS_HASH {
return Err(anyhow!("Remote verification only works with mainnet. Please omit the --remote flag to verify locally."));
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you forgot to add this get_genesis_hash check back in

@ngundotra
Copy link
Collaborator

Looking at this again, I think this may also solve an error that occurs if you attempt to run verify-from-repo --remote before running verify-from-repo locally.

This error will be thrown when attempting to fill --library-name when none is provided. We attempt to run find in a repo, but that repo will not exist on user's file system until we clone the repo. Which does not happen on --remote.

Would appreciate if you could check that --remote works even if you haven't run verify-from-repo locally for that url

Copy link
Collaborator

@ngundotra ngundotra left a comment

Choose a reason for hiding this comment

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

Ln 1140 has get_genesis_hash check

@ngundotra ngundotra merged commit 8f7c260 into Ellipsis-Labs:master Dec 18, 2024
8 checks passed
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.

2 participants