Don't use macros when a function would do.

[MicahZoltu]

Macros are a higher risk language construct compared to functions. In particular, they can reference local variables in a disassociated way that makes for an easy path to introduce subtle bugs. For example, if you have a function with a local variable foo then call into a macro that references foo. If you change in the function that makes foo mean something different or changes its expected usage pattern, it is easy to miss the fact that the macro has direct access to that local variable foo and may mutate or read it under certain (now incorrect) expectations. With a function, it is very clear what parameters are being passed at the call site so you don't need to worry about the called function accessing/mutating state in a surprising way.

bidAndAsk.se is a good example of a situation where functions would have solved the problem just as well as macros, but without the risks.

In general, my recommendation is to only use macros for very small code snippets (a few lines) that are called in a lot of places (e.g., refund()) and don't reference any variables that aren't passed in. Any code groupings that are larger or need access to anything other than the passed variables should be a function instead.

[tinybike]

Function calls cost something in Serpent, so macros can legitimately be substituted for functions. However, if macros are used this way, I strongly agree that they should be explicitly passed variables that they are intended to access and/or mutate. (This is done in some places but not others at the moment -- e.g., saveOrder is explicitly passed its arguments, but its sub-macros placeBid and placeAsk just use saveOrder's local variables without being passed anything.)

[MicahZoltu]

Are you referring to the gas cost for function calls? If so, I would argue that this is a premature/unnecessary optimization. A CALL operation costs 700 gas. This is on the order of magnitude of a log operation @ 375 gas or an SLOAD operation @ 200 gas. I would strongly urge not falling into the trap of doing gas micro-optimizations like this until it is shown that gas costs are too high, at which point hot-path analysis should be done to target the problem areas rather than moving everything over to macros. Most of the non-trivial macros I have seen so far are not in a hot path, some are even only called in once place (not in a loop).

[tinybike]

To give you some background on this (as I understand it) the contracts are structured primarily to minimize gas costs. Gas cost minimization has been prioritized over everything except security, including "code quality" issues like readability, dryness, etc. Changing this design principle would be @joeykrug's decision, although I'd argue it's pretty late in the game to make a change like this.

[MicahZoltu]

Gas cost minimization has been prioritized over everything except security

Code quality issues are a security issue. Especially for mutable code (one of the things Augur is shooting for). Things like readability, ease of understanding, DRYness, etc. are all quite important in keeping code secure, especially as it mutates with time.

If you accept this assertion, then that means gas optimization is really the last priority and should only be undertaken if it can be done so in a way that doesn't decrease code quality or it is critically necessary for Augur to function (e.g., gas costs so high that no one uses Augur is a problem).

If you don't accept this assertion, then that is a separate topic for debate. There are studies (can't find the source paper) that have shown this correlation, and it aligns with my personal experience. Poor code quality is a top fuzzy-contender for security vulnerabilities, following "change".

As with all security issues, one needs to do a risk analysis to decide if the benefits outweigh the costs. In this particular case (IMO), the 700 gas cost for a function call does not outweigh the security risks introduced by using macros over functions with the exception of some very simple and common macros (like refund).

[joeykrug]

Using functions over macros actually adds duplicate code, requires fetching and setting variables unnecessarily, adds repetition, and "With a function, it is very clear what parameters are being passed at the call site so you don't need to worry about the called function accessing/mutating state in a surprising way." isn't actually a problem that's solved by using functions alone but by also fetching as much as the state that is needed before a function or macro call, then passing the information in as parameters.

The contracts are not structured to minimize gas costs, in fact currently I expect they are far too high for many of the functions as is. They are structured to have minimal features above all else [and still again probably have too many], because pretty much every other issue is aided by less features. This minimizes gas cost as a strong byproduct.

As far as input validation bugs, that's something good to check and input is validated rather heavily throughout the contracts.

[joeykrug]

I think this issue has merit in some cases where macros are used one off, so leaving it open. [large one off macros also seems to result in gas issues with loading the contracts I've found out, so some of the worst offenders will be fixed soon]

[tinybike]

Heretical suggestion: cut-and-paste one-off macros into the functions they're called from. (Code expansion, the old fashioned way!)

[joeykrug]

That'd make things a lot more difficult to understand and involve a lot of repetition of code in the files themselves imo

[tinybike]

Even if it was just for one-off (i.e., only-used-once) macros?

[joeykrug]

Oh for one off ones probably not as big an issue. There are a handful of one offs that should be separate functions imo though. Good example of this is fillBid and fillAsk should really just be functions that trade calls instead of macros, actually, if you put them in the same file/contract it won't even compile

[MicahZoltu]

Using functions over macros actually adds duplicate code

I believe this is only true if you are using macros like generics in other languages, which I don't think applies in this context (since serpent is dynamically typed)?

adds repetition

Can you expand on this? How do functions result in more code duplication over macros?

With a function, it is very clear what parameters are being passed at the call site so you don't need to worry about the called function accessing/mutating state in a surprising way.

isn't actually a problem that's solved by using functions alone but by also fetching as much as the state that is needed before a function or macro call, then passing the information in as parameters.

You can try to solve this problem with coding standards (e.g., never reference a local variable from a macro, always use $ variables). However, in general it is better to solve problems with a computer than to rely on humans to behave a certain way. A bug can be as simple as someone accidentally forgetting the $ on a macro variable named value and all of a sudden the macro does something that it really shouldn't have. You can't make this mistake in a function, because the compiler enforces function scoping.

[AlexeyAkhunov]

What about extending serpent compiler to support inline functions? :) And while we are at it, I would also add "with" statement for mutexes

[tinybike]

@AlexeyAkhunov I think extending with to support mutexes would be terrific! Using with for context-with-automatic-cleanup is a great convenience in Python; would be great to see that in Serpent as well.