-
Notifications
You must be signed in to change notification settings - Fork 157
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
Add voyager verification to the verify
command
#2566
base: master
Are you sure you want to change the base?
Add voyager verification to the verify
command
#2566
Conversation
verify
command
3b1f764
to
4c5d105
Compare
Addressing some of @integraledelebesgue comments from #2288
Seems that its failing to compile without the macro. Any alternatives you might have had in your mind?
Sure
|
Successfully made some of the methods default, and remove the need of using a base struct inside each concrete verification interface struct. Since the verification provider api is constructed slightly differently, I don't think that replacing Will move on to update the current tests tomorrow. |
d978d39
to
70532da
Compare
Tests updated! Reviews would be appreciated. |
cc @cptartur |
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.
Hey, thanks for contributing. I think some parts may be improved.
Also, I haven't reviewed the tests yet but will do soon.
CHANGELOG.md
Outdated
#### Added | ||
- Add Voyager API support for `verify` subcommand [Read more here](https://foundry-rs.github.io/starknet-foundry/appendix/sncast/verify.html). | ||
|
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.
#### Added | |
- Add Voyager API support for `verify` subcommand [Read more here](https://foundry-rs.github.io/starknet-foundry/appendix/sncast/verify.html). | |
#### Added | |
- Voyager API support for `verify` subcommand [Read more here](https://foundry-rs.github.io/starknet-foundry/appendix/sncast/verify.html). | |
|
||
#[async_trait] | ||
pub trait VerificationInterface { | ||
fn get_workspace_dir(&self) -> Utf8PathBuf; |
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 don't think this should be a part of trait. I think all verification providers will want to handle it the same so there's no point abstracting it like that.
I'd simply change verify
signature so it takes this directory as well.
Additionally read_workspace_files
should be removed from this trait and should be made a free function that takes directory as an argument.
async fn send_verification_request( | ||
&self, | ||
url: String, | ||
payload: VerificationPayload, | ||
) -> Result<VerifyResponse> { | ||
let json_payload = serde_json::to_string(&payload)?; | ||
let client = reqwest::Client::new(); | ||
let api_res = client | ||
.post(url) | ||
.header("Content-Type", "application/json") | ||
.body(json_payload) | ||
.send() | ||
.await | ||
.context("Failed to send request to verifier API")?; | ||
|
||
if api_res.status() == StatusCode::OK { | ||
let message = api_res | ||
.text() | ||
.await | ||
.context("Failed to read verifier API response")?; | ||
Ok(VerifyResponse { message }) | ||
} else { | ||
let message = api_res.text().await.context("Failed to verify contract")?; | ||
Err(anyhow!(message)) | ||
} | ||
} |
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 isn't using self
anyway, I'd just make it a normal function.
#[serde( | ||
default, | ||
rename( | ||
serialize = "verification-base-url", | ||
deserialize = "verification-base-url" | ||
) | ||
)] | ||
/// Custom base url to be used for verification | ||
pub verification_base_url: Option<String>, |
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 want to allow overriding the URL, I think it should be done only in verify
subcommand arguments not in a global configuration.
@@ -7,4 +7,4 @@ pub mod multicall; | |||
pub mod script; | |||
pub mod show_config; | |||
pub mod tx_status; | |||
pub mod verify; | |||
pub mod verification; |
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 wouldn't rename that. Modules in starknet_commands
generally follow the name of the commands they are related to.
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.
Gotcha, will revert this.
pub trait VerificationInterface { | ||
fn get_workspace_dir(&self) -> Utf8PathBuf; | ||
|
||
fn gen_explorer_url(&self, config: CastConfig) -> String; |
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 instead of having an config
argument, implementations should take a base url and network arguments when being created with new
.
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 way usage is much simpler: provide an relevant override of gen_explorer_url
that forms correct urls from information you have and verify
can use it without providing anything.
|
||
The address of the contract that is to be verified. | ||
|
||
## `--class-name <CONTRACT_NAME>` |
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 are we renaming this?
// ensure that either contract_address or class_hash is provided | ||
if contract_address.is_none() && class_hash.is_none() { | ||
bail!("Either contract_address or class_hash must be provided"); | ||
} |
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 this can be done at clap level with ArgGroup
and flatten
Take a look at this: https://stackoverflow.com/questions/76315540/how-do-i-require-one-of-the-two-clap-options
## `--class-hash, -c <CONTRACT_ADDRESS>` | ||
Required. | ||
|
||
The address of the contract that is to be verified. |
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.
Let's also say that this conflicts with the other argument and exactly one is required.
70532da
to
4a9f0fd
Compare
Addressed partially, will continue in awhile to address the rest. |
This is a follow up PR to complete changes that #2288 wants to introduce.
Closes #2287
Introduced changes
Additional context on a possible edge case
As Voyager backend limits the http alive time to less than 30s, given a large contract which compilation might take longer, we might have to have the compilation task run asynchronously in order and have the status polled. This is currently not implemented in this PR as testing is being done to see if this is significant enough of a problem to be addressed this way.
Checklist
CHANGELOG.md
Will complete this checklist after another self review.