# dmusic review

## dmusic-backend

1. `README.md`
- Running `make image`:
- missing info about requirement of `node` `yarn` `npx` installed on your machine.
- missing info about requirement of [mvn](https://maven.apache.org/run.html) installed on your machine.
- i'm blocked - it fails on `npl-maven-plugin`

  - generally speaking it's an antipattern to require installation of global dependencies, especially when part of the repo is dockerized.

2. [`casper-connector`](https://github.com/NoumenaDigital/dmusic-backend/tree/master/casper-connector)
    - at the first sight I recommend to use some linting & formatting tools - [JavaScript Standard Style](https://standardjs.com/) is popular, ready to use solution. Another one is eslint + prettier.
    - [storing secret keys in a repo](https://github.com/NoumenaDigital/dmusic-backend/tree/master/casper-connector/files) is for sure an anti-pattern and unsafe.
    - [casperConnector.ts#L17](https://github.com/NoumenaDigital/dmusic-backend/blob/master/casper-connector/src/casper/casperConnector.ts#L17) - not a good practice to hardcode some external URL even as a fallback.
    - [casperConnector.ts#L18](https://github.com/NoumenaDigital/dmusic-backend/blob/master/casper-connector/src/casper/casperConnector.ts#L18) - same as above, network name shouldn't be hardcoded - I would throw an error when it is missing.
    - [casperConnector.ts#L19](https://github.com/NoumenaDigital/dmusic-backend/blob/master/casper-connector/src/casper/casperConnector.ts#L19) - usused variable.
    - [casperConnector.ts#L21-L24](https://github.com/NoumenaDigital/dmusic-backend/blob/master/casper-connector/src/casper/casperConnector.ts#L21-L24) - unused variable.
    - [casperConnector.ts#L26-L27](https://github.com/NoumenaDigital/dmusic-backend/blob/master/casper-connector/src/casper/casperConnector.ts#L26-L27) - unused variable.
    - [casperConnector.ts#L29-L33](https://github.com/NoumenaDigital/dmusic-backend/blob/master/casper-connector/src/casper/casperConnector.ts#L29-L33) - unused variable.
    - [casperConnector.ts#L38-L50](https://github.com/NoumenaDigital/dmusic-backend/blob/master/casper-connector/src/casper/casperConnector.ts#L38-L50) - you don't need to parse a secret keys as a files - you just need one private key as an env variable - this is how to parse it as a string (typescript example):

import {
Keys,
decodeBase64
} from "casper-js-sdk";

enum KEY_VARIANTS {
ED25519,
SECP256K1,
}

export const getKeysFromHexPrivKey = (
key: string,
variant: KEY_VARIANTS
): Keys.AsymmetricKey => {
const rawPrivKeyBytes = decodeBase64(key);
let keyPair: Keys.AsymmetricKey;

if (variant === KEY_VARIANTS.SECP256K1) {
const privKey = Keys.Secp256K1.parsePrivateKey(rawPrivKeyBytes);
const pubKey = Keys.Secp256K1.privateToPublicKey(privKey);
keyPair = new Keys.Secp256K1(pubKey, privKey);
return keyPair;
}

if (variant === KEY_VARIANTS.ED25519) {
const privKey = Keys.Ed25519.parsePrivateKey(rawPrivKeyBytes);
const pubKey = Keys.Ed25519.privateToPublicKey(privKey);
keyPair = Keys.Ed25519.parseKeyPair(pubKey, privKey);
console.log(`Public key hex: ${keyPair.publicKey.toHex()}`);
return keyPair;
}

throw Error("Unsupported key type");
};

    - [casperConnector.ts#L60](https://github.com/NoumenaDigital/dmusic-backend/blob/master/casper-connector/src/casper/casperConnector.ts#L60) - you're using this connector with multiple contracts at the same time? if so, this is potentially unsafe to change the contract hashes in a runtime and it may cause some race condition issues. My suggestion is to set `contractHash` in a constructor and use one instance of `CasperConnector` to handle one contract at a time.
    - [casperConnector.ts#L62](https://github.com/NoumenaDigital/dmusic-backend/blob/master/casper-connector/src/casper/casperConnector.ts#L62) - `publicKey` is not a good name, maybe `owner` or `recipient` would be better.
    - [casperConnector.ts#L84](https://github.com/NoumenaDigital/dmusic-backend/blob/master/casper-connector/src/casper/casperConnector.ts#L84) - it's oversimplification - there potentialy might be more invalid params than just contract hash.
    - [casperConnector.ts#L94](https://github.com/NoumenaDigital/dmusic-backend/blob/master/casper-connector/src/casper/casperConnector.ts#L94) - same as before, multi contract usage? 
    - [casperConnector.ts#L113](https://github.com/NoumenaDigital/dmusic-backend/blob/master/casper-connector/src/casper/casperConnector.ts#L113) 
    - [casperConnector.ts#L170](https://github.com/NoumenaDigital/dmusic-backend/blob/master/casper-connector/src/casper/casperConnector.ts#L170) - no need to recreate an `CasperClient` instance everytime on a function call. Would move it to constructor.