AztecProtocol / aztec-packages

Apache License 2.0
187 stars 189 forks source link

Improve constructor syntax check #3630

Open drgorillamd opened 10 months ago

drgorillamd commented 10 months ago

TL;DR: wrong constructor syntax should be caught and have a proper error message, same as a missing constructor.

Rationale is, due to context switching with Solidity or JS (which I guess is to expect), these are difficult errors to catch, especially with the misleading compute_note_hash_and_nullifier error (Rustaceans should be fine tho).

Shamim06 commented 10 months ago

Thanks for pointing this out! Having clearer error messages for incorrect constructor syntax, like #[aztec(private)], and enforcing constructor presence during compilation would greatly improve the Solidity development experience. These enhancements could streamline debugging and benefit developers, especially when switching contexts between languages. Improving error handling in Solidity aligns with creating a more user-friendly environment. Your insights are valuable for enhancing the language’s usability and reliability!

drgorillamd commented 10 months ago

ike #[aztec(private)],

Should have added (but I guess it's a nice way to demonstrate it), here, the issue is a missing fn

e: and the "error handling" is in Noir, not Solidity... GitHub spam?

rahul-kothari commented 10 months ago

Okay so there are really two fixes:

  1. Noir fix: for a syntax error, error message is unhelpful. E.g. if you forget adding the word "fn", you get Expected an end of input but found contract
  2. Aztec-nr fix: throw a compile time error if constructor is missing. It is only currently caught at deploy time today.

Thanks for flagging this @drgorillamd

drgorillamd commented 10 months ago

@rahul-kothari For 1., the expected out of input at least point in the right direction of a syntax error, the really big pain is if it throws compute_note_hash_and_nullifier function not found (too aggressively hunting for such?), which mislead to an implementation issue (ergo pain, despair, tears, etc)

rahul-kothari commented 10 months ago

Jyst added a compile time error if constructor is missing. Noir team will be looking into better function errors

drgorillamd commented 10 months ago

Great! ty! Commented the commit @rahul-kothari , maybe we can catch both issues there (I know, a tad hacky tho)