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

feat: add FromStr to ZkSolcWarnings and ZkSolcErrors #37

Merged
merged 1 commit into from
Nov 5, 2024

Conversation

elfedy
Copy link

@elfedy elfedy commented Nov 4, 2024

This is required to parse suppressed error/warnings from cli arguments, so users can pass for example --zk-suppressed-warnings txorigin while also failing when passing an invalid value (Else we need to either silently pass nothing or panic)

@Karrq
Copy link

Karrq commented Nov 4, 2024

Hmm I'd opt to store this as strings actually, and pass them untypedly to the compiler.
For now we have only these 2 but perhaps in the future more will be added (or removed), so I'd prefer to keep this decoupled

@elfedy
Copy link
Author

elfedy commented Nov 4, 2024

Hmm I'd opt to store this as strings actually, and pass them untypedly to the compiler.
For now we have only these 2 but perhaps in the future more will be added (or removed), so I'd prefer to keep this decoupled

@Karrq I guess the place to discuss this was this previous PR and not here? @hedgar2017 your call probably on this though since we will probably end up using that common crate for zksolc settings. I don't have strong feelings either way.

@nbaztec
Copy link

nbaztec commented Nov 5, 2024

As a user if I run --help I expect to see the options I can use, and for that it always makes sense that libraries provide the concrete types which are string serializable, and not just free form text.

However, if we expect this field to change quite a bit we can implement it as a pass-through, but that would be the anti-pattern.

@hedgar2017
Copy link
Collaborator

Hey @elfedy - I've moved everything including these structs to this crate.
Please let me know if you need anything to start using it!
There can be some derives missing, so feel free to drop them there with a quick PR.

Copy link
Collaborator

@hedgar2017 hedgar2017 left a comment

Choose a reason for hiding this comment

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

Approving for now for the sake of unblocking, but please start migrating to our common definitions!

@elfedy elfedy changed the base branch from zksync-v0.11.4 to zksync-v0.11.6 November 5, 2024 16:59
@elfedy elfedy merged commit 7db9a2e into zksync-v0.11.6 Nov 5, 2024
4 checks passed
@elfedy elfedy deleted the elfedy-suppressed-fromstr branch November 5, 2024 17:01
Karrq pushed a commit that referenced this pull request Nov 26, 2024
feat: add FromStr to ZkSolcWarnings and ZkSolcErrors (#37)
Karrq added a commit that referenced this pull request Nov 26, 2024
* feat: add zksolc support

* fix: codeowner change (#32,#34)

* feat: zksolc v1.5.7 support (#35)

* feat: zksolc 1.5.7 support

* chore: update version constants

* feat: Support error / warning suppression in zksolc (#33)

feat: add FromStr to ZkSolcWarnings and ZkSolcErrors (#37)

* fix: add back skip filter over avoid-contracts

* feat(zk): properly mark unlinked contract (#40,#41,#43,#44)

feat(zk): mark bytecode as unlinked
refactor(zk): get output from `eravm` object
feat: retain pre-1.5.7 zksolc compatibility
test(artifact:zk): unprefixed bytecode test

* chore: bump version

* chore: merge conflicts 11.6->12.3

feat(zk): per-source profile & settings
feat(zk): stub our zksolc settings restrictions

* fix(ci): update rust toolchain version

* fix(ci): proper rust toolchain name

* chore: clippy

---------

Co-authored-by: Federico Rodríguez <[email protected]>
Co-authored-by: Tomi Moreno <[email protected]>
Co-authored-by: Alex Ostrovski <[email protected]>
Co-authored-by: Dustin Brickwood <[email protected]>
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.

4 participants