Review of NChain by ConsenSys Mesh and contributors

[humbitious]

Nchain
Reviewer: Sam Stokes
Documentation
Github Repo
No instructions for running tests locally. Should be included in the README.md
Would be nice to see a sequence diagram/flow diagram showing how different Provide microservices interact with each other over NATS
README.md: docs link is broken
CONTRIBUTING.md: section 5 - links to pics in s3 are broken
Public Gitbook
Bridges: no documentation provided. Says “Documentation forthcoming”
Connector: “A Connector is an adapter that connects external arbitrary infrastructure with Provide”
Need explanation of how this is accomplished to understand the use-case
Consensus: which protocols and blockchains are supported by this feature?
Oracles: missing documentation for sections “List Oracles”, “Create Oracles”, “Retrieve Oracle Details”
Tokenization: no documentation provided. Says “Documentation forthcoming”
Code Review
Manual
Makefile
timeout: command does not exist on osx therefore some scripts crash
Tests
test/ directory is empty (delete this directory since tests are located elsewhere?)
I do not see a test coverage report so not sure how much of the code gets tested
“make integration_ropsten_short” tests pass. However, several containers report a status of “unhealthy” while the tests are running

Wallet package
account.go, line 85: commented code should be removed, uncommented, or have an explanation why it is commented out
Wallet.go: why use dependency "github.com/jinzhu/gorm" instead of “https://github.com/go-gorm/gorm”?
Prices package
prices.go: What are PRVD tokens? What are they used for? What mainnet will they be hosted on?
Line 13: // PRVD tokens are worthless until the mainnet launches
Tokens package
token.go: When would a Provide user want to create a new token?
It appears that the token is only created in the Postgres db. Is there a listener to that db that triggers a token smart contract deployment?
Tx package
Does this package process both pre-EIP1559 and post-EIP1559 transactions?
nonce.go: Several “TODO” and “HACK” comments that need attention
Integration package
artifacts/: What is “Ekho protocol” referenced in ekho.json? What is it used for within the Provide platform?
Filters package
handlers.go: InstallFilterAPI is not implemented. What is its purpose?
Connectors package
What are connectors used for? Are you trying to hash database records and store the hashes on a blockchain?
If you create a connector for a database, will updates to records in that database be broadcast to counterparties?
Should SAP be one of the Providers?
Zokrates is listed as a provider but not Gnark. Will gnark be added at some point?

Static Analyzer
The static analyzer was run against “dev” branch on commit 44a2ae9f9ddcbfa000fd2ebe2e63efccc26c8a2d

Performance
Comment: Use COPY instead of ADD for files and folders
Files: Dockerfile

Security
Comment: Use of weak cryptographic primitive (consider using more secure alternative to MD5)
Files: consumer/packet_reassembly.go (lines 414, 281, 127, 90)
Comment: Potentially bad TLS connection settings detected
Files:
connector/provider/zokrates.go (line 319)
connector/provider/ipfs.go (line 70)
Comment: Import blacklist: crypto/md5
Files: consumer/packet_reassembly.go
Comment: Audit the random number generation source (rand). Recommend using ‘crypto/rand` instead
Files: network/network.go (lines 657, 634)

Anti-pattern
Comment: Trapping a signal that cannot be trapped
Files
cmd/statsdaemon/main.go (line 53)
cmd/reachabilitydaemon/main.go (line 47)
cmd/consumer/main.go (line 48)
cmd/api/main.go (line 110)
Comment: Use plain channel send or receive
Details: Select statements with a single case can be replaced with a simple send or receive. If you intend to handle the case when there is no value received from channel, add a default case to make the select statement non-blocking.
Files: cmd/migrate/main.go (line 104)
Comment: Empty body in an if or else block
Files
tx/tx.go (line 275)
connector/provider/zokrates.go (line 212)
Comment: Unused shell script variable
Files
ops/run_local_tests_debug.sh
NCHAIN_API_PATH
NCHAIN_API_SCHEME
IDENT_API_PATH
VAULT_API_PATH
NATS_TOKEN
NATS_CLUSTER_ID
NATS_STREAMING_CONCURRENCY
GIN_MODE
REDIS_HOSTS
ops/run_local_tests.sh
NCHAIN_API_PATH
NCHAIN_API_SCHEME
NATS_TOKEN
NATS_CLUSTER_ID
LOG_LEVEL
REDIS_HOSTS
ops/ecs_deploy.sh
revNumber (line 6)
ECR_IMAGE (line 86)
Comment: trap code will expand during definition. With double quotes, all parameter and command expansions will expand before trap execution. Using single quotes will prevent expansion at declaration time, and save it for execution time.
Files: ops/await_tcp.sh (line 61)

Potential Bugs
Comment: the result of append is not used anywhere
Files: filter/filter.go (line 72, 83)
Comment: A value assigned to a variable is never read before being overwritten
Files
wallet/account.go (line 235)
tx/funcless.go (line 150)
network/consumer.go (line 163)
filter/filter.go (line 83, 72)
connector/provider/sql.go (line 264)
connector/handlers.go (line 229, 195)
cmd/migrate/main.go (line 131)
Comment: Pin versions in apk add. Instead of apk add <package> use apk add <package>=<version>
Files: Dockerfile (line 11)
Comment: Always tag the version of an image explicitly
Files: Dockerfile (line 9)
Comment: Compilation errors
Files:
network/orchestration/azure.go (line 189, 221, 233)
Comment: Expressions don't expand in single quotes, use double quotes for that
Files:
ops/run_local_tests_short.sh (line 231, 235, 240)
ops/run_local_tests_long.sh (line 219, 223, 228)
Comment: Use double quote to prevent globbing and word splitting
Files:
ops/ecs_deploy.sh (line 6, 92)
ops/run_local_tests_debug.sh (line 226)
ops/run_local_tests.sh (line 234)
ops/run_consumer_container.sh (line 191)
ops/run_api_container.sh (line 191)
ops/ci_process.sh (line 63, 64)
ops/await_tcp.sh (line 36, 39, 56, 58, 146)
Comment: Remove surrounding $() to avoid executing output
Files: ops/ecs_deploy.sh (line 73)
Comment: Unused code
Files
tx/tx.go (line 1527, 235)
tx/shuttle_consumer.go (line 23)
tx/nonce.go (line 56)
tx/handlers.go (line 183)
tx/consumer.go (line 41, 36)
network/p2p/bcoin.go (line 9, 13, 14)
network/node.go (line 29, 31, 32, 36, 38, 41, 42, 44, 45, 46, 47, 49, 51, 55, 63, 239, 280, 285, 777, 786, 795)
network/network.go (line 27, 28, 29, 34, 41, 60, 706)
db/database.go (line 15)
consumer/packet_reassembly.go (line 21)
consumer/consumer.go (line 27)
connector/provider/rest.go (line 52, 66)
connector/provider/redis.go (line 52, 66)
connector/provider/nats.go (line 52, 66)
connector/provider/elasticsearch.go (line 55, 69)
common/utils.go (line 16, 17)
common/globals.go (line 20)
cmd/statsdaemon/stats_daemon.go (line 34, 35, 36, 40, 525, 766)
cmd/statsdaemon/log_transceiver.go (line 28, 254)
cmd/statsdaemon/historical_block_daemon.go (line 48, 65, 72, 120, 459)
cmd/reachabilitydaemon/reachability_daemon.go (line 48, 162)