search

dap-ps / discover

Discover a whole new world of curated decentralised applications
https://dap.ps
Mozilla Public License 2.0
21 stars 9 forks source link

Discover Rebuild #87

Open RyRy79261 opened 3 years ago

RyRy79261 commented 3 years ago

Tl;Dr:

This overhaul implements Redux-Saga for improved state & business logic management, code generators, theming engine, caching & performance improvements as well as token data syncing for future development.

Front end changes

The entire webapp from the ground up has been rebuilt, all is easily customisable and future changes should be more straight forward.

The rebuild was aimed to allow the app to be mobile responsive from the get go.

Data flow changes

Onchain state has been prioritised, while caching server has had no modifications, additionally, data is pulled from the target network regardless of browser's Web3 capabilities or selected network.

Potential improvements

Useful changes

Contracts

All contract functions are now selfcontained callable functions.

Potential improvements

Theming

Theming is currently controlled via Material-Ui, theme.ts contains all the theming constants & colors used in the app

Potential improvements

Performance

Potential improvements

DX (Developer Experience)

A number of changes have been made to improve the overall experience of updating & developing in Discover:

Potential improvements

Important notes

Environment variables

Some modification to the environment variables have been made, .env.example can be found in each of the sections Backend, Contracts & WebApp

Issues with existing Embark configuration

I experienced odd behavior from Embark in deploying contracts to ropsten & setting the addresses in the output Embark contract files, as such, work arounds have been implemented while using the output from the existing Embark section of the code base

Build considerations

There is an issue (potentially only present on my machine) where wsrun doesn't carry over the NODE_ENV env variable correctly, thus to build, please update your .env settings as needed, then run yarn build

The build in ./WebApp outputs a zipfile containing the built webapp files, which are ready to run from the root of a serving http server

I am unsure of the requirements or usage of some of the commands within the Contracts & Backend section, changes might need to be made based on feedback

jakubgs commented 3 years ago

There's so many changes here we might as well start a new branch using git checkout --orphan or even a new repo.

jakubgs commented 3 years ago

Seriously, this looks like a rewrite more than an update. A new repo would make more sense.

jakubgs commented 3 years ago

I guess the contract stuff stays the same, but still. I wish there was "mark all renamed as viewed" or something.

RyRy79261 commented 3 years ago

@jakubgs

I do agree that potentially a new repo would be preferable as then I can implement a more explicit mono repo.

My biggest hurdle in this rebuild was getting embark to function with the rest of the flow.

Build local for example I couldnt get to function and the contract side was out-of my initial scope

As such my contract addresses are bundled from environment variables instead of the embark config as the contracts that embark would deploy had the wrong constructor parameters leading to reverts.

richard-ramos commented 3 years ago

Should yarn run build:local deploy to testnet or to mainnet?

Right now, the the package.json for Contracts contains: "build": "embark build livenet" meaning that when wsrun command in the root package.json executes, it will attempt to deploy the contract DiscoverKyberSwap to mainnet instead of ropsten (due to the livenet parameter of embark,

How can I create a build pointing to testnet/development?

richard-ramos commented 3 years ago

I wonder if the Embark issues are product of using v5.2.3. Perhaps they were fixed in 6.0.0 :thinking:

RyRy79261 commented 3 years ago

@richard-ramos As far as the contracts are concerned, I couldn't get anything beyond compiling to function as expected.

I created a work around by deploying manually via remix to get around incorrect constructor issues, I then pull the addresses from environment variables at build

Building for prod: yarn create:fullbuild Running local: yarn start:dev

richard-ramos commented 3 years ago

This PR should fix the issue with Embark deployment: https://github.com/RyRy79261/discover/pull/1 I was able to execute embark run testnet and have the contracts deployed (after removing them from the testnet {} section of contracts.js

RyRy79261 commented 3 years ago

Thanks @richard-ramos I see that library: 'embarkjs', is specified as the library, is Ethers an option? I used it in lieu of Embark for the rebuild

jakubgs commented 3 years ago

I'm still failing to build the app with yarn build:local due to:

 | [DiscoverKyberSwap]: Returned error: Returned error: insufficient funds for gas * price + value
 | Error deploying contracts. Please fix errors to continue.

And using yarn start:dev fails on page load with;

 | Error: ENOENT: no such file or directory, stat '/home/sochan/work/dapps:Backend/discover/Backend/frontend/index.html'
RyRy79261 commented 3 years ago

@jakubgs Seems theres a pathing issue on your machine, build local is deprecated as I am unsure what its initial purpose was in the repo since it dealt with the embark side of things

jakubgs commented 3 years ago

@RyRy79261 yeah, we'll need to clean up the legacy targets from package.json and Makefile after this is moved to a separate repo.