General improvements for utils/upgradeability/compare_variables_order.py

[EricR]

• Fixed slither-check-upgradeability raises "IndexError: list index out of range" #274
• Refactored code
• Made logging more explicit and (hopefully) clearer

[montyly]

Nice work!

So the loop now in compare_variables_order_proxy will first print out all the variable in the proxy, in the previous version, it was printing all the variables, but with a warning when something was wrong.

Typically, we would have:

- var1
- var2 -> warning something
- var3

Now we have

- var1
- var2 
- var3

- var2 -> warning something

I think the previous order was a bit better, as we reduce the size of the output. The main issue though that the info was not clear enough.

Maybe we should keep the original logic, but refactor the output to be more clear?

[EricR]

Nice work!

So the loop now in compare_variables_order_proxy will first print out all the variable in the proxy, in the previous version, it was printing all the variables, but with a warning when something was wrong.

Typically, we would have:

- var1
- var2 -> warning something
- var3

Now we have

- var1
- var2 
- var3

- var2 -> warning something

I think the previous order was a bit better, as we reduce the size of the output. The main issue though that the info was not clear enough.

Maybe we should keep the original logic, but refactor the output to be more clear?

I reworked the logic a little bit in an attempt to fix some confusing checks. For example, if there are more variables in version 1 of a contract than version 2, the check will currently report that variables are missing when they may just be out of order:

slither/utils/upgradeability/compare_variables_order.py

Lines 32 to 36 in 1128b4c

	for idx in range(0, len(order_v1)):
	(v1_name, v1_type) = order_v1[idx]
	if len(order_v2) < idx:
	logger.info(red('Missing variable in the new version: {} {}'.format(v1_name, v1_type)))
	continue

Similarly, if version 2 of a contract has more variables than version 1, the check will currently report that variables are new when they may just be out of order:

slither/utils/upgradeability/compare_variables_order.py

Lines 46 to 49 in 1128b4c

	if len(order_v2) > len(order_v1):
	new_variables = order_v2[len(order_v1):]
	for (name, t) in new_variables:
	logger.info(green('New variable: {} {}'.format(name, t)))

I agree that the repeated variables in output isn't great. What if instead we did the following?

    for idx in range(len(vars_v1)):
        (v1_type, v1_name) =  vars_v1[idx]
        if idx > len(vars_v2):
            logger.info(red('Expected variable in {}: {} {}'.format(
                new_name, v1_type, v1_name
            )))
            issues_found = True
            continue

        (v2_name, v2_type) =  vars_v2[idx]
        if (v1_name != v2_name) or (v1_type != v2_type):
            logger.info(red('Expected variable in {}: {} {}. Found {} {} instead.'.format(
                new_name, v1_type, v1_name, v2_type, v2_name
            )))
            issues_found = True
        elif warn:
            logger.info(yellow('Variable in {}: {} {}'.format(new_name, v2_type, v2_name)))

This way we're simply detecting when each variable in v1 isn't where it was expected to be in v2. More like a diff. Then we can just display a list of truly new variables (in v2 but not v1) after the ordering warnings.

[montyly]

I am closing this PR. It is out of date, and will be replaced by #354