search

Byte-Masons / Reliquary

50 stars 20 forks source link

Add support to typescript #4

Closed StErMi closed 2 years ago

StErMi commented 2 years ago

I haven't made any PR because I'm external to the team but I would like to give my 2cents.

Having support to typescripts help a lot in two things:

1) you're not blind when you write tests, everything is automatically suggested and typesafe. The tradeoff on slowness when you run test/build is not so much a huge deal compared to the benefits 2) it allows you to generate typescript-ready interfaces SDK for your frontend applications easily distributable as an npm package (so external services can install them as versioned dependencies).

I have a solidity-template project that is easily forkable for that but it's really a matter of fact of creating a tsconfig, updating the hardhat config file to .ts (and updating the content), and adding some dependencies like 

@typechain/ethers-v5
@typechain/hardhat
typechain

If you want to follow my solidity-template you can head over here: https://github.com/StErMi/solidity-template

StErMi commented 2 years ago

didn't wanted to just to throw the issue and run, for me there's no problem to implement both this and #5 if you are interested and you don't mind

Zokunei commented 2 years ago

for sure, I think they're great proposals. you should have write access soon if you don't already

Zokunei commented 2 years ago

if you want to push a branch for this it would be most welcome

StErMi commented 2 years ago

I have created a branch to view typescript migration. I'm not familiar with the contracts and Reaper flow so please, double and triple-check them. I've not changed anything from the contract, just made some typescript migrations and config files.

Repo Link: https://github.com/Byte-Masons/Reliquary/tree/typescript_and_more

TODO (need more info)

Packages added

Configurations & Others

Prettier

I've added my prettier rules, these are totally a personal preference so please bear in mind that. I've not applied them to .sol files just because I didn't want to create merge conflicts atm.

Further improvements

Before doing other changes would be important to understand where these things are used and what impact they could have

I've not added them because I see no deploy scripts but these could be useful

    "deploy": "npx hardhat run --network localhost scripts/deploy.ts",
    "deploy:staging": "npx hardhat run --network test scripts/deploy.ts",
    "deploy:production": "npx hardhat run --network opera scripts/deploy.ts",
Zokunei commented 2 years ago

I would like to first say that I'm confident the merge had no negative impact on master. We can migrate the other files to Typescript in a later commit.

I think moving ReaperSDK to its own repo is a great idea and will discuss it with others. Not sure if we can migrate it to Typescript without having to rewrite every script that ever used it though. The json files are only used by ReaperSDK, so they might have to be moved to that repo.

ChefTester.js is basically a script for testing Reliquary that uses console.logs instead of asserts. It seems to only work when using testnet or (presumably) mainnet.

Tesseract has already been running prettier, so you could ask him about the settings to use to avoid creating too many changes.

StErMi commented 2 years ago

I would like to first say that I'm confident the merge had no negative impact on master. We can migrate the other files to Typescript in a later commit.

cool!

I think moving ReaperSDK to its own repo is a great idea and will discuss it with others. Not sure if we can migrate it to Typescript without having to rewrite every script that ever used it though. The json files are only used by ReaperSDK, so they might have to be moved to that repo.

as far as I know, there's no problem in using TS lib with vanilla js code. In the end, TS is just transpiled to JS anyway when you're using the library it just helps who's using it in a TS project ;)

Ping me out

ChefTester.js is basically a script for testing Reliquary that uses console.logs instead of asserts. It seems to only work when using testnet or (presumably) mainnet.

Ok, so I can migrate also it without a problem.

Tesseract has already been running prettier, so you could ask him about the settings to use to avoid creating too many changes.

@tess3rac7 ping me on discord to discuss my prettier and solidity prettier config. as I said that was my personal preference but we can manage to create a standard.

Would also be cool to create a package for it so every project just installs eslint/prettier and init them with the bytemason config that can also be shipped as a package.

@Zokunei let me know when you have some time to discuss those remaining JS files. Would like to have a brief overview because at the moment, for example, I can't do anything TS related because ReaperSDK is deploying contracts that do not exist in the project (for example UniswapV2Factory).

0xBebis commented 2 years ago

i support this effort, will leave it in the hands of you and @Zokunei

Zokunei commented 2 years ago

TypeScript is no longer needed as all tests/scripts are through Foundry.