Local development

[tobyclemson]

This PR adds a comprehensive local build that:

• installs dependencies
• compiles
• lints and formats the contracts
• lints and formats the tests
• runs the tests, automatically starting and stopping ganache in the background

This local build should act as a pre-commit build to check that a push is mergeable. Instructions are in the README.

A few things to note:

• solhint didn't like the method naming convention for payment methods, e.g., ..._ETH_TKN_... since it breaks naming conventions as defined at https://docs.soliditylang.org/en/develop/style-guide.html#function-names. I've renamed across the board to be camel case and this will require a redeployment and some changes to the app frontend.
• There are a number of outstanding solhint warnings that I need some help with. I'd like to get down to zero warnings either by fixing or by ignoring. Right now, the build is set to allow the 32 warnings and no more but ideally this should be set to 0.
• The test files are currently huge and I feel in need of breaking up. 2_test_fullpath_with_permit.js is sitting at >4200 lines after prettier's formatting.

I'm not sure I understand the reason for the numeric prefixes on the test files. I think their names could be a bit more explanatory / consistent.

For reference, the outstanding warnings are:

contracts/BosonRouter.sol
  166:9  warning  Provide an error message for require  reason-string
  324:9  warning  Provide an error message for require  reason-string
  362:9  warning  Provide an error message for require  reason-string
  524:9  warning  Provide an error message for require  reason-string
  575:9  warning  Provide an error message for require  reason-string

contracts/Cashier.sol
  454:17  warning  Possible reentrancy vulnerabilities. Avoid state changes after transfer  reentrancy

contracts/ERC20WithPermit.sol
   39:9   warning  Avoid to use inline assembly. It is acceptable only in rare cases  no-inline-assembly
  128:29  warning  Avoid to make time-based decisions in your business logic          not-rely-on-time

contracts/VoucherKernel.sol
    27:1   warning  Contract has 17 states declarations but allowed no more than 15  max-states-count
   217:29  warning  Avoid to make time-based decisions in your business logic        not-rely-on-time
   474:63  warning  Avoid to make time-based decisions in your business logic        not-rely-on-time
   511:63  warning  Avoid to make time-based decisions in your business logic        not-rely-on-time
   557:21  warning  Avoid to make time-based decisions in your business logic        not-rely-on-time
   565:21  warning  Avoid to make time-based decisions in your business logic        not-rely-on-time
   572:70  warning  Avoid to make time-based decisions in your business logic        not-rely-on-time
   590:21  warning  Avoid to make time-based decisions in your business logic        not-rely-on-time
   596:21  warning  Avoid to make time-based decisions in your business logic        not-rely-on-time
   601:70  warning  Avoid to make time-based decisions in your business logic        not-rely-on-time
   615:17  warning  Avoid to make time-based decisions in your business logic        not-rely-on-time
   659:21  warning  Avoid to make time-based decisions in your business logic        not-rely-on-time
   665:71  warning  Avoid to make time-based decisions in your business logic        not-rely-on-time
   669:21  warning  Avoid to make time-based decisions in your business logic        not-rely-on-time
   679:21  warning  Avoid to make time-based decisions in your business logic        not-rely-on-time
   685:21  warning  Avoid to make time-based decisions in your business logic        not-rely-on-time
   694:17  warning  Avoid to make time-based decisions in your business logic        not-rely-on-time
   756:32  warning  Avoid to make time-based decisions in your business logic        not-rely-on-time
   789:17  warning  Avoid to make time-based decisions in your business logic        not-rely-on-time
   798:13  warning  Avoid to make time-based decisions in your business logic        not-rely-on-time
   808:17  warning  Avoid to make time-based decisions in your business logic        not-rely-on-time
   816:17  warning  Avoid to make time-based decisions in your business logic        not-rely-on-time
  1144:39  warning  Avoid to make time-based decisions in your business logic        not-rely-on-time
  1145:37  warning  Avoid to make time-based decisions in your business logic        not-rely-on-time

[thecryptofruit]

@HristiyanG I suggest I prepare a payout table for "all" scenarios in a spreadsheet, so we can have full coverage of paths. Would you be able to pick it up from there and add scenarios in test/?

[HristiyanG]

@thecryptofruit sounds like a great idea. I already have implemented some withdrawal scenarios in #16 . Have a look and advice what's missing. I am closing this one as i do not think this is required anymore.

[tobyclemson]

Hey Hris,

This one's still needed and still WIP. Will get to it next week.

Thanks,
Toby

[thecryptofruit]

I'm not sure I understand the reason for the numeric prefixes on the test files. I think their names could be a bit more explanatory / consistent.
@tobyclemson , are you talking about /test directory? The numbers are interpreted to enforce testing order, I think.

[thecryptofruit]

Great stuff! 👍

[tobyclemson]

I'm not sure I understand the reason for the numeric prefixes on the test files. I think their names could be a bit more explanatory / consistent.

@tobyclemson , are you talking about /test directory? The numbers are interpreted to enforce testing order, I think.

Ok, if there are dependencies between tests that require them to run in a particular order, I think we should remove the dependencies as tests are intended to be independent. I noticed within the test files, there are a number of suite level variables that each test used. IMO this is a smell as you end up with cross test coupling.

I think this is probably for a separate PR though.

[tobyclemson]

Anything else needed to merge?