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

Voyager verification support #3093

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

jkopanski
Copy link
Contributor

@jkopanski jkopanski commented Mar 13, 2025

Closes #2287

This will submit the contract for verification successfully, but the verification itself will fail as we need to release backend changes for supporting this. Those are in review right now.

Introduced changes

Checklist

  • Linked relevant issue
  • Updated relevant documentation
  • Added relevant tests
  • Performed self-review of the code
  • Added changes to CHANGELOG.md

@jkopanski jkopanski force-pushed the voyager-verify branch 2 times, most recently from fc33b1f to 1ad6a90 Compare March 14, 2025 12:13
@jkopanski jkopanski marked this pull request as ready for review March 14, 2025 12:41
@jkopanski jkopanski requested a review from a team as a code owner March 14, 2025 12:41
Comment on lines +20 to +23
### Voyager

[Voyager](https://voyager.online/) is the Starknet block explorer.

Copy link
Contributor

@franciszekjob franciszekjob Mar 23, 2025

Choose a reason for hiding this comment

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

Shouldn't we mention possibility of setting VOYAGER_API_URL (same for walnut btw)?

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 was mimicking walnut here, but since both aren't mentioned anywhere right now, perhaps we should name it something like VERIFIER_API_URL for both?

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct me if I'm wrong, but this will enforce to update the env every time you want to use different verifier? I know it may be a rare case, but theoretically that's how it would look.

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'm not sure what you mean here.

I think the users would select verifier using command line arguments. I suspect the env variable was added for easier testing and targeting local verifiers, so it's kind of like a developer mode.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what you mean here.

I mean that if there will be VERIFIER_API_URL (shared between both verifiers), you'll have to change it every time you change verifier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but change verifier in what sense? Users change verifier through command line argument, the env variable is left unset almost all the time.

Copy link
Contributor

@franciszekjob franciszekjob Mar 27, 2025

Choose a reason for hiding this comment

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

but change verifier in what sense?

Let's say you use walnut and you've set VERIFIER_API_URL to walnut one. Then you switch to voyager, hence you need to change the env again (because env is shared). I know it may be a rare case. Wdyt?

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 know it may be a rare case. Wdyt?

Yes it's rare, because (as a sncast user) in order to use walnut you don't set any env variable. You pass --verifier walnut.

The env variable acts as an override so a developer (either walnut, voyager or foundry dev) could target non production instance. IMHO then having single env var makes more sense, but it's not a hill I'm willing to die on ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, I'm leaving this up to you 😄

Copy link
Member

@kkawula kkawula left a comment

Choose a reason for hiding this comment

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

This will submit the contract for verification successfully, but the verification itself will fail as we need to release backend changes for supporting this. Those are in review right now.

  • Can you link this issue/pr?

  • Please add entry there - docs/src/appendix/sncast/verify.md

@jkopanski jkopanski force-pushed the voyager-verify branch 2 times, most recently from 349c9e2 to d6d3cef Compare March 25, 2025 11:38
@franciszekjob
Copy link
Contributor

@jkopanski Is it ready to re-review?

@jkopanski jkopanski requested a review from franciszekjob March 28, 2025 12:33
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.

Add voyager verification interface to the verify command
3 participants