refactor verification crate & add voyager interface #2288
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rjnrohit
requested review from
Arcticae,
Draggu,
ksew1 and
tomek0123456789
as code owners
rjnrohit
changed the title
integraledelebesgue
requested changes
Aug 8, 2024
| base: BaseVerificationInterface, | ||
| } | ||
|
|
||
| #[async_trait] |
There was a problem hiding this comment.
#[async_trait] attribute is not required
|
|
||
| #[derive(Parser)] | ||
| #[command(about = "Verify a contract through a block explorer")] | ||
| pub struct Verify { |
There was a problem hiding this comment.
Please add descriptions of all parameters to display in help
| } | ||
|
|
||
| fn gen_explorer_url(&self) -> Result<String> { | ||
| let api_base_url = env::var("VOYAGER_API_URL") |
There was a problem hiding this comment.
We would like to unify the wole configuration to a TOML file. Could you please redesign the url acquisition logic to use an enum provied via CastConfig rather than an environmental variable?
| #[async_trait] | ||
| pub trait VerificationInterface { | ||
| fn new(network: Network, workspace_dir: Utf8PathBuf) -> Self; | ||
| async fn verify( |
There was a problem hiding this comment.
Both implementations of this method are identical. You can make it a default implementation
| class_hash: Option<FieldElement>, | ||
| class_name: String, | ||
| ) -> Result<VerifyResponse>; | ||
| fn gen_explorer_url(&self) -> Result<String>; |
There was a problem hiding this comment.
- Similarly to
verify, implementations of this method differ only with the url source. How about introducing a new trait method, let's sayget_base_url, implementing it manually on both types and using in a default implementation ofgen_explorer_url? - This method never fails. Are there any reasons for it to return a
Result?
|
Hi @rjnrohit! Please let us know if you are still working on these features. |
|
Closing because of lack of activity. |
5 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #2287
Introduced changes
Add voyager verifier to the
sncastverify command.Checklist
CHANGELOG.md