jibrelnetwork / jibrel-contracts Goto Github PK

Mark all possible functions as external

https://solidity.readthedocs.io/en/latest/contracts.html#visibility-and-getters

Be kind, free blockchain space, remove unused contracts

• short description
• links to the website and repo with whitepapers
• badge coveralls
• How to test
  • npm test
  • jsapi test
  • coverage test

It is worth to configure coveralls for our project. Should be straightforward, we always have scripts for this (copied from zeppelin-solidity repo).

Add coveralls badge to the readme

Merge code for coverage tests into the branch dev
Hit the 100% coverage

If we will try to create really fast API of a blockexplorer - we will face a problem: in order to read smth from the contract state - we have to call getter of the contract.
I.e. we have to launch Ethereum virtual machine and execute the contract.
It is not possible to cache such data in the blockexplorer.
A possible solution - save state in the events.
For example, when user transfer tokens - we save in the events not only addresses of sender&receiver and transferred amount, but also new balances of each of them.

• Think about this
• Estimate gas costs
• If we agree with this solution - implement

returns (address varName) -> returns (address)

Logging

• save data to the file

Tests

• Switch to jsapi completely

JSAPI

• create logging decorators
• chek inputs

Fix errors produced by eslint

16c1853#diff-e2e926dfee4892578903faaa7075ea92

Transmission of ETH does not make sense for crydr view&storage contracts.
Do we need to declare them and set restrictions for ETH transfers.
For example, we can allow only to crydr controller to send ETH to the contract.
Or even completely prohibit transmission of ETH to the contracts

Currently crydrs allow to send 1 tokens
I.e. A can send 0 jUSD to B, and A will be charged in JNT
This does not make sense
Abb check value > 0

It is a very common need for tokens - be able to block transfers of just one account.
We need to add it before crydrs published
I.e.

function blockAccount(address _account) onlyManager('block_accounts') {
    blockedAccounts[_account] += 1;
}
function unblockAccount(address _account) onlyManager('unblock_accounts') {
    blockedAccounts[_account] 1= 1;
}

Also, there is another common need for accounts - block some amount of account:

function blockFunds(address _account, uint _value) onlyManager('block_funds') {
    blockedFunds[_account] += _value;
}

If 500 tokens blocked - balance of account can not go below 500

One crydr - one migration script
It will be difficult (in case of big migration script) to manually revive the system if smth wrong with the migration

When we will have hundreds of Cydrs - we will have a really big risk to mess with view/storage/controller contracts. For example, we can assign controller of CRYDR#1 to the view of CRYDR#2
We need a reliable way to eliminate such risks.

We can do the following:

• assign the unique ID to each crydr
• hardcode this id to each view/storage/controller contract
• check these IDs when the manager tries to unpause contract

Truffle uses names of contracts as unique ID
To avoid mess we have to make versioning using names of contracts

There is a mess in our tests
We need to:

• create a test suite for each contract
• use test suite for contract and all contract derived from it

Sometimes Ethereum node returns undefined for the request.
In this situation, whole migration is failed
need to skip a number of failed requests to the node, and only this fail

CrydrStorageBaseInterface.sol
add events for accouns/funds blocks

Actually, this applies to any method that is not expose directly to the users

Lib is finished. Merge tests for this lib into the branch dev

And update to Solidity 0.4.17

https://github.com/ethereum/EIPs/blob/master/EIPS/eip-20-token-standard.md

check names of events params
it is different from zeppelin-solidity

Strings are easier to read, less error prone

We can pass any bytecode to the contract and execute it using contract`s private key
This is will be used for non-ordinary cases.
For example, when someone accidentally sends ERC20 tokens to our contracts and we have to rescue this funds.
It worth to this functionality to crydr views&storages - contracts which we are not able to replace with the newer version

function executeBytecode(address target, uint ethValue, bytes transactionBytecode)  onlyAllowedManager('..') {
         isExecuted = true; // Avoid recursive calling
         require(target.call.value(ethValue)(transactionBytecode));
         isExecuted = false;
         // fire event
    }

Prohibit to a specific address to receive tokens

We need to eliminate possibility of short address attack

Many functions are guarded by the same require statement such as require(getAccountBlocks(_msgsender) == 0); here, here and here. It is recommendable to move such repetitive statements into modifiers.

http://chaijs.com/api/assert/#method_strictequal

Check implementation of a token in zeppelin-solidity library
Copy tests for tokens if we missed something

Same for Pausable contract

https://github.com/jibrelnetwork/jibrel-contracts/blob/master/contracts/crydr/controller/CrydrControllerBase.sol#L19

We need to implement two-step process:

• old owner request transfer ownership to the new owner
• new owner creates tx to actually become an owner

This saves from the wide range of bugs related to migration scripts

Evaluation of gas amount used to deploy a contract does not work on the main network

Add tests for crydr controllers

web3 does not work correctly with the events that have indexed parameters with variable size (strings, arrays etc.).
We should set all such params as not indexed.
I.e.:

event ManagerPermissionGrantedEvent(address indexed manager, string indexed permission); -> event ManagerPermissionGrantedEvent(address indexed manager, string permission);