WIP: Eugen/refacto contracts

[eugenPtr]

Question: Is there a solid reason why the deals array should be kept private?

Something that's currently missing from the tests is verifying the deal status (see tests/Swapper.js lines 130 and 149)

• One solution would be able to directly access the deals array and fetch the status from there.
• Another way to do it is include the deal status in the emitted events and fetch it from there.

Looking forward to feedback on both the contract refactoring and the tests.

[Willgg]

Question: Is there a solid reason why the deals array should be kept private?

By default all the variables should be private. This is considered as a best practice in smart contract programming. The best pattern is to keep variables private but call them in public functions so we can use modifiers to restrict access to specific users. Like so :

mapping(address => uint256) private _balances;

and

 function balanceOf(address account) public view virtual override returns (uint256) {
     return _balances[account];
 }

This code is from openZeppelin ERC20. People can inherit from this contract and then override the balancedOf by adding modifiers:

 function balanceOf(address account) public view override(ERC20) onlyOwner {
     super.balanceOf();
 }

Also there is no reason why someone should be able to get all the deals. We just need to provide functions so people can swap token, everything that is not dedicated to this task has nothing to do in our contract. Our smart contract must do one job: Swapping token. This is not a database dedicated to provide free available data for everyone.

So I rephrase you question: Is there a solid reason why the deals array should be kept public ?

To test the status you can check the event triggered in the function that changes the deal status. Like approve().

await expect(contract.approve({id}))
      .to.emit(contract, "DealApproved")
      .withArgs({id}, {msg.sender}, {deal.proposer1}, {deal.proposer2});

Just replace with the correct variable what is inside {}

We could also use a function state(uint256 id) to check for the state of a specific deal but remember that each byte of code cost more eth to deploy so if we don't really need a function to swap tokens we should not implement it. (Front-end can check state through events)

[eugenPtr]

Thanks for taking the time to explain all these. It makes sense.

Can you see any flaws in the implementation?