Prośba o review portfolio do kryptowalut

Cześć. Uczę się Reacta i NodeJSa. Zrobiłem w ramach tej nauki proste portfolio do kryptowalut. Najpierw trzeba przejść przez login system w NodeJS, który łączy się z bazą danych MySQL. Trzeba podać nazwę użytkownika, maila (może być zmyślony), hasło i potwierdzenie hasła. W przypadku nie wypełnienia któregoś z pól, formularz zrobiony przy pomocy MUI wyświetla komunikat. Jeśli wszystkie pola są wypełnione, to przed wysłaniem requesta sprawdzam w React, czy oba podane hasła są takie same. Jeśli nie, to wyświetla się błąd z animacją pojawiania i znikania. Jeśli wszystko gra, to idzie request do backendu i w response, jeśli e-mail nie jest zajęty, przychodzi potwierdzenie rejestracji. Jeśli email jest zajęty, to wyświetla się podobny błąd do tego od confirm password. Po udanej rejestracji można się zalogować. Tutaj analogicznie do sprawdzenia emaila przy rejestracji, backend sprawdza czy hasło pasuje do użytkownia. Jeśli tak, to następuje logowanie. Od razu po zalogowaniu idzie request do API Coingecko, z którego pobieram nazwy wszsystkich kryptowalut i chowam je do tablicy. Jest to potrzebne do zrobienia propozycji w dropdownie przy wpisywaniu nazwy kryptowaluty. Wpisywanie nazwy kryptowaluty to właśnie pierwsza z czynności, którą trzeba zrobić. Po wybraniu nazwy należy wpisać ilość i cenę, po jakiej kupiło się wpisaną kryptowalutę. Przy wciskaniu przycisku 'add transaction' następuje walidacja. Jeśli wpisana kryptowaluta nie istnieje, albo w ilości lub cenie zostało wpisane coś innego niż liczba większa od -1, to transakcja zostaje dodana do bazy MySQL przed backend w NodeJS, tak samo jak przy rejestracji użytkownika. Po otrzymaniu potwierdzenia z backendu, że transakcja została dodana, lista transakcji na dole się aktualizuje, a następnie pobierane są aktualne ceny z Coingecko dla wszystkich kryptowalut użytkownika i liczony jest zysk (albo strata) dla każdej transakcji z osobna i ogólny zysk. Takie samo przeliczenie następuje od razu po zalogowaniu, jeśli użytkownik ma już jakieś transakcje dodane podczas innej sesji. Wszystko jest liczone w PLN. Transakcje można usuwać. W .htaccess zrobiłem przekierowania www/non-www itd na jeden adres, żeby nie było problemów z CORS. Aplikacja chodzi na hostingu MyDevil. Reactowe pliki są zwyczajnie wrzucone przez FTP, a backend w NodeJS jest odpalony przez "forever" w CLI.

Aplikacja powstała w celach niekomercyjnych. Chętnie usłyszę opinie dotyczące użytkowania aplikacji i jakości jej kodu. Chciałbym jak najwięcej się nauczyć, więc piszcie śmiało co sądzicie o czymkolwiek związanym z tą apką, zachęcam do dyskusji.

Adres: https://crypto.vyost.usermd.net
Github: https://github.com/MarekSzczepanski/react-crypto-portfolio

Pozdrawiam. :)

JS jako backend i to jeszcze jak chce się robić operacje na pieniądzach to pierwszy i drugi XD.
(JS nie umie w liczby bo to język do wyświetlania, a node.js jest pierońsko dziurawy i IMO nie nadaje się do krypto).

Jak logujesz error to w JS są od tego zdefiniowane funkcje np. console.err().

Ogolnie backend i front trzymbym w osobnych repo.

Użyłeś strasznie "ciężkiej" bazy danych by trzymać takie dane. Nie lepiej jakąś dokumentowa, albo klucz-wartosc?

Z tego co widzę to masz dodawanie, pobieranie i usuwanie. Gdzie usuwanie powinno być zabronione chyba, bo na tym opierają się kontrakty. Dane kontraktu też powinny być zahashowane i powinny trzymać hash poprzednika.

Ogólnie wydaje mi się, że nie licząc nazwy, twój projekt z crypto nie ma nic wspólnego

Dregorio napisał(a):

JS jako backend i to jeszcze jak chce się robić operacje na pieniądzach to pierwszy i drugi XD.
(JS nie umie w liczby bo to język do wyświetlania, a node.js jest pierońsko dziurawy i IMO nie nadaje się do krypto).

Jak logujesz error to w JS są od tego zdefiniowane funkcje np. console.err().

Ogolnie backend i front trzymbym w osobnych repo.

Użyłeś strasznie "ciężkiej" bazy danych by trzymać takie dane. Nie lepiej jakąś dokumentowa, albo klucz-wartosc?

Z tego co widzę to masz dodawanie, pobieranie i usuwanie. Gdzie usuwanie powinno być zabronione chyba, bo na tym opierają się kontrakty. Dane kontraktu też powinny być zahashowane i powinny trzymać hash poprzednika.

Ogólnie wydaje mi się, że nie licząc nazwy, twój projekt z crypto nie ma nic wspólnego

Chyba pomyliłeś portfolio z portfelem. Moje portfolio to wyłącznie apka do śledzenia zysków poszczególnych transakcji, a nie do ich przeprowadzania. :D Użyłem Reacta, NodeJSa i MySQLa dlatego, że chciałbym pracować w tych technologiach, więc robię w nich projekty. To w 100% ćwiczebny i niekomercyjny program.

Tak od początku to

1) package.json - możesz rozdzielić paczki na dependencies i devDependencies

{
  ...
  "dependencies": {
    "@emotion/react": "^11.10.4",
    "@emotion/styled": "^11.10.4",
    "@mui/icons-material": "^5.10.6",
    "@mui/material": "^5.10.8",
    "bcryptjs": "^2.4.3",
    "cors": "^2.8.5",
    "dotenv": "^16.0.2",
    "express": "^4.18.1",
    "mysql": "^2.18.1",
    "react": "^18.2.0",
    "react-dom": "^18.2.0",
  },
  "devDependencies": {
    "@testing-library/jest-dom": "^5.16.5",
    "@testing-library/react": "^13.4.0",
    "@testing-library/user-event": "^13.5.0",
    "nodemon": "^2.0.20",
    "react-scripts": "5.0.1",
    "web-vitals": "^2.1.4"
  }
  ...
}

2) controllers/transactions.js - Taki zapis SELECT * FROM transactions WHERE user_id = ${req.query.id} wygląda mi na SQL Injection

export const fetch = (req, res) => {
  db.query(`SELECT * FROM transactions WHERE user_id = ${req.query.id}`, async (err, results) => {
    // ...
  });
}

export const remove = (req, res) => {
  db.query(`DELETE FROM transactions WHERE id = ${req.query.id}`, async (err, results) => {
    //
  });
}

3) src/index.js - Przy imporcie skryptów

import App from './App.jsx';, końcówki js / jsx / ts / tsx nie są wymagane

więc można to zapisać tak

import App from './App'

4) src/App.css - Ładowanie czcionki w pliku css powinno być teoretycznie wolniejsze niż jakbyś zrobił to w sekcji head. Mówię "teoretycznie", bo istnieją bundlery, które potrafią to zoptymalizować, ale w tym wypadku raczej nic takiego nie ma.

5) src/components/AddTransactionForm.jsx - Zamiast tej dziwnej pętli wewnątrz funkcji, powinieneś wydzielić to jako osobny komponent, lub zapisać wynik do stanu i dopiero na podstawie tego zrobić pętle

const renderCurrencyNameProposals = () => {
    if (!namesProposals) return false;
    
    let proposals = [];
    for (let i=0; i<namesProposals.length; i++) {
      if (i > 10) break;
      if (currencyName !== namesProposals[i].name) {
        proposals.push(<div className='proposal' key={i} data-currency_id={namesProposals[i].id} onClick={(e) => handleProposalClick(e)}>{namesProposals[i].name}</div>);
      } 
    }
    return proposals;
};

<div className='autocomplete'>
    <TextField id='currency-name' label='Crypto name' variant='standard' type='text' value={currencyName} autoComplete="off" onChange={(e) => handleNameInputChange(e)} onBlur={(e) => e.relatedTarget ? setNamesProposals(null) : null}/>
    {namesProposals ? renderCurrencyNameProposals() : null}
</div>

6) src/components/TransactionsContainer.jsx - Tworzenie komponentów Mui wewnątrz drugich komponentów też nie jest zbyt dobre, bo będzie się to niepotrzebnie wykonywać przy każdym renderowaniu. Ten fragment powinien być przeniesiony poza komponent

const StyledTableCell = styled(TableCell)(({ theme }) => ({
    [`&.${tableCellClasses.head}`]: {
        backgroundColor: theme.palette.common.black,
        color: theme.palette.common.white,
    },
    [`&.${tableCellClasses.body}`]: {
        fontSize: 14,
    },
}));
  
const StyledTableRow = styled(TableRow)(({ theme }) => ({
    '&:nth-of-type(odd)': {
        backgroundColor: theme.palette.action.hover,
    },
    '&:last-child td, &:last-child th': {
        border: 0,
    },
}));

res.send({registered: true});

Raczej status powinien informować o wykonaniu operacji. Tworzysz nowy obiekt w zasobie users i zwracasz kod http. W przypadku create - 201. Jeśli błąd to np. 500. Informacje co poszło nie tak np. mail zajęty, też możesz dołączyć do responsa.

Rejestruję się na zajętego maila i dostaję status 200 OK. No nie tak powinno to działać.

{"name":"dsaasd","email":"[email protected]","password":"test","confirmPassword":"test"}

Nie trzeba haseł z obydwu inputów wysyłać na back. To zwykłe stringi i na froncie można sprawdzić, czy są takie same.

Niezabezpieczony endpoint z transakcjami https://crypto.vyost.usermd.net:60332/transactions/fetch?id=5.

Xarviel napisał(a):

"mysql": "^2.18.1",

(...)
2) controllers/transactions.js - Taki zapis SELECT * FROM transactions WHERE user_id = ${req.query.id} wygląda mi na SQL Injection

Tutaj jest opisane, jak się przed tym bronić: https://www.npmjs.com/package/mysql#escaping-query-values
możesz podać placeholder values albo connection.escape

Jako, że dość uważnie śledzę crypto, dla mnie taka apka, żeby była dla mnie użyteczna, powinna być bardzo ciężka na backendzie.
Konkretnie:

• Powinieneś scrapować giełdy (a nie serwisy typu coingecko)
• Trzymać timeseries z dużą częstotliwością z jak największej liczby giełd
• Mieć cache ostatnich wartości i wykresów z timeseriesów dla predefiniowanych okresów (żeby apka śmigała!!!!); cache z jak najmniejszym eviction time (może nawet co 1s dla każdej crypto)
• Mieć możliwość podejrzenia wartości/volume na każdej z giełd, na której dane crypto występuje

Zostanę userem apki, która spełni powyższe wymagania :D

BTW. info o transakcjach w ogóle mnie nie interesuje. Interesuje ile, czego, i gdzie mam.
Przykładowa apka, która nie jest tragiczna, to Hodler. Dużo researchu nie robiłem, więc nie znam za bardzo rynku, ale na Hodlerze poprzestałem, bo byłem zrozpaczony tym, że większość takich apek to są klasy pracy zaliczeniowej na studiach.

https://github.com/MarekSzczepanski/react-crypto-portfolio/blob/2b23fcf03a9a8f36678aec426492df3d9f713de3/src/components/LoginForm.jsx

const email = document.getElementById('login-email').value;
const password = document.getElementById('login-password').value;

Gubisz sens używania Reacta.
Jeśli chcesz element DOM, to lepiej użyć useRef https://reactjs.org/docs/hooks-reference.html#useref
Ale w tym przypadku jak łapiesz zdarzenie onSubmit, to pod e.target masz formularz, a pod e.target.elements masz poszczególne elementy indeksowane po nazwie.
https://developer.mozilla.org/en-US/docs/Web/API/HTMLFormElement/elements

Tak możesz spróbować:

const email = e.target.elements.email.value;
const password = e.target.elements['login-password'].value;

alternatywnie mógłbyś też używać kontrolowanych komponentów i te dane trzymać na poziomie Reacta
https://reactjs.org/docs/forms.html#controlled-components

natomiast document.getElementById('login-email')nie ma tutaj wiele sensu, bo sens używania Reacta (i innych podobnych bibliotek) jest taki, żeby pozwolić na tworzenie niezależnych komponentów, które można sobie wrzucić na stronę niezależnie od kontekstu, nawet po kilka razy. A tego nie wrzucisz kilka razy, bo jak będziesz na twardo mieć id, to już drugi taki sam komponent ci się rozwali (bo tylko jeden element może mieć to samo id). id czasem się przydaje, ale tutaj nie ma to wiele sensu.