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.

Sign up for GitHub

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

Jump to bottom

Support for the verification with class hash #2906

Merged
merged 26 commits into from
Mar 28, 2025
Merged

Support for the verification with class hash #2906

merged 26 commits into from
Mar 28, 2025

Conversation

marijamijailovic
Copy link
Contributor

@marijamijailovic marijamijailovic commented Jan 31, 2025 •
edited by cptartur
Loading

Closes #2668

Introduced changes

This PR adds support for sncast verify --verifier walnut to use class hash.

Checklist

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

kkawula
kkawula reviewed Jan 31, 2025
View reviewed changes
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.

Please add tests for new logic

@marijamijailovic
Copy link
Contributor Author

Hi @kkawula, thanks for your comments! I have addressed them. Additionally, I have removed the "Verify contract" section from sncast/README.md due to a failed test (changing the --network parameter to --url "http://127.0.0.1:5055"), as Walnut does not support the --url parameter.

kkawula
kkawula approved these changes Feb 11, 2025
View reviewed changes
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.

Please add an entry in CHANGELOG.md

@marijamijailovic marijamijailovic requested a review from a team as a code owner February 11, 2025 08:59
@marijamijailovic
Copy link
Contributor Author

Please add an entry in CHANGELOG.md

Hey @kkawula, thanks for your comments. I added entry to the CHANGELOG.md.

@marijamijailovic
Copy link
Contributor Author

Hey @kkawula, I’ve updated my changes with the latest changes from the master branch. Let me know if there’s anything else needed.

cptartur
cptartur requested changes Feb 28, 2025
View reviewed changes
@marijamijailovic
Copy link
Contributor Author

Hi @cptartur , thanks for your comments! I have addressed them and update my branch so it's up to date with master.

cptartur
cptartur reviewed Mar 13, 2025
View reviewed changes
@marijamijailovic
Copy link
Contributor Author

Thank you for all your previous comments! I've updated the branch based on the feedback. Could you please take another look and let me know if it's ready for merging? Appreciate your time!

@cptartur
Copy link
Member

Thank you for all your previous comments! I've updated the branch based on the feedback. Could you please take another look and let me know if it's ready for merging? Appreciate your time!

@marijamijailovic good on my side, let's wait for @kkawula review

kkawula
kkawula reviewed Mar 24, 2025
View reviewed changes
@cptartur cptartur added this pull request to the merge queue Mar 25, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Mar 25, 2025
@marijamijailovic
Copy link
Contributor Author

Hey @cptartur, I see that this PR has been removed from the merge queue. Should I resolve the merge conflict?

@cptartur
Copy link
Member

Hey @cptartur, I see that this PR has been removed from the merge queue. Should I resolve the merge conflict?

@marijamijailovic yes, please.

@marijamijailovic
Copy link
Contributor Author

Hey @cptartur, I see that this PR has been removed from the merge queue. Should I resolve the merge conflict?

@marijamijailovic yes, please.

I have updated branch

cptartur
cptartur reviewed Mar 28, 2025
View reviewed changes
@cptartur cptartur enabled auto-merge March 28, 2025 13:17
@cptartur
Copy link
Member

@marijamijailovic I think you need to resolve conflicts again and it will start merging

auto-merge was automatically disabled March 28, 2025 14:06

Head branch was pushed to by a user without write access

@marijamijailovic
Copy link
Contributor Author

@marijamijailovic I think you need to resolve conflicts again and it will start merging

Hey, @cptartur I resolved the conflicts.

@cptartur cptartur added this pull request to the merge queue Mar 28, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 28, 2025
@cptartur cptartur added this pull request to the merge queue Mar 28, 2025
Merged via the queue into foundry-rs:master with commit 6364553 Mar 28, 2025
37 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cptartur cptartur cptartur approved these changes

@kkawula kkawula Awaiting requested review from kkawula

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support of --class-hash for Walnut verification provider
3 participants
@marijamijailovic @cptartur @kkawula