Best Practices and Audit Fixes (Smart Contracts)

Ricochet-Exchange / ricochet-protocol

Other

26 stars 32 forks source link

Best Practices and Audit Fixes (Smart Contracts) #32

Closed rashtrakoff closed 2 years ago

rashtrakoff commented 2 years ago

The issue is regarding styling and optimisation of smart contracts. Currently, the contracts aren't following best practices according to out style guide as well as linter. Also in some contracts, modest optimisations can be made such as converting some variables to constants and cutting down on number of parameters passed during initialisation. Further description will be provided in the PR comments regarding the changes made.

harshitsinghai77 commented 2 years ago

We can add linter and other style practices in the Github Action. This will make sure the best practices are followed every time the PR is merged with the main branch.

josealonso commented 2 years ago

@harshitsinghai77, I'm sure @rashtrakoff is doing his best to make sure the solidity code doesn't have any warnings and follows the best practices. I have my own rules in my linter, too. There are a lot of warnings right now. But he's still improving and understanding @mikeghen's code. It's a work in progress.

rashtrakoff commented 2 years ago

We can add linter and other style practices in the Github Action. This will make sure the best practices are followed every time the PR is merged with the main branch.

Yes, we actually discussed about this but I think using CI, we can probably only add linter and prettier tasks. Other style practices that I am talking about is a bit more abstract so it really depends on the creator of a contract. I think for CI, having linter and prettier check is enough @harshitsinghai77

rashtrakoff commented 2 years ago

This issue is tabled for later. Until all contracts are reviewed and stable, this issue will exist.

josealonso commented 2 years ago

@rashtrakoff, I assume this will be merged as soon as v.2 contracts are deployed, am I right ? Did you talk to @samirsalem about integrating this tasks in the CI tool ?

rashtrakoff commented 2 years ago

@rashtrakoff, I assume this will be merge as soon as v.2 contracts are deployed, am I right ? Did you talk to @samirsalem about integrating this tasks in the CI tool ?

I guess you are right about merging this after V2 deployment but as the contracts have changed significantly since I created this PR/issue, the contracts need to be revisited.

Regarding CI, no I haven't but I think it's just the matter of checking for linter errors and warnings.

josealonso commented 2 years ago

Retaking this task, right after seeing the test suite have been migrated to sdk-core .

josealonso commented 2 years ago

The plan is:

Make the contracts smaller
Use a single contract thanks to the Clone factory pattern
In the mid-term, make it upgradeable. Before all those steps, having a clean code with no warnings and a comprehensive test suite in sdk-core is very advisable. The scope of this issue is having a clean code and make the contracts smaller
```
the contracts need to be made smaller first, right now they're all ~24KB
```

mikeghen commented 2 years ago

Closed because of inaction