Skip to content

Refactor#38

Merged
gupadhyaya merged 45 commits intoharmony-one:mainfrom
johnwhitton:refactor
Jun 9, 2022
Merged

Refactor#38
gupadhyaya merged 45 commits intoharmony-one:mainfrom
johnwhitton:refactor

Conversation

@johnwhitton
Copy link
Contributor

@johnwhitton johnwhitton commented May 25, 2022

Refactor to improve solidity tooling using hardhat including testing, deployment, documentation and gas estimation. As well as consolidate all tools, scripts and cli into a src folder.

Migration Strategy

  • Smart Contract use Hardhat with Typescript and ethers(instead of web3)
    • Replace all web3 with ethers
    • replace all solidity deployments and test js files with typescript
    • remove all truffleConfig use hardhat instead
    • write tests
  • docs: new folder for documentation
  • docs/assets => migrated from assets
  • docs/solidity: contains generated solidity documentation
  • deploy: new folder for deployment scripts (using hardhat-deploy and logic from scripts)
  • src: new folder for typescript source files
  • src/lib: (migrated from scripts)
  • src/cli: (migrated from cli)
  • src/(elc, eprover, eth2hmy-relay): migrated from tools(elc, eprover, eth2hmy-relay)

Action Items

  • Resolve gas issue with Kovan Deployment
  • Resolve upgrade Process
  • Generate DAGs for Local Ethereum (Hardhat) node

Logs and Analysis

johnlaptop horizon (refactor) $ npx hardhat deploy --network hardhat --tags HarmonyLightClient
Nothing to compile
No need to generate any newer typings.
✅ Generated documentation for 46 contracts
(node:62043) UnhandledPromiseRejectionWarning: Error: processing response error (body="{\"jsonrpc\":\"2.0\",\"id\":42,\"error\":{\"code\":-32600,\"message\":\"invalid request\"}}\n", error={"code":-32600}, requestBody="{\"method\":{\"jsonrpc\":\"2.0\",\"method\":\"hmyv2_getFullHeader\",\"params\":[\"0xe\"],\"id\":1654537992451},\"id\":42,\"jsonrpc\":\"2.0\"}", requestMethod="POST", url="http://localhost:9500", code=SERVER_ERROR, version=web/5.6.0)
    at Logger.makeError (/Users/john/one-wallet/horizon/node_modules/@ethersproject/logger/src.ts/index.ts:261:28)
    at Logger.throwError (/Users/john/one-wallet/horizon/node_modules/@ethersproject/logger/src.ts/index.ts:273:20)
    at /Users/john/one-wallet/horizon/node_modules/@ethersproject/web/src.ts/index.ts:329:28
    at step (/Users/john/one-wallet/horizon/node_modules/@ethersproject/web/lib/index.js:33:23)
    at Object.next (/Users/john/one-wallet/horizon/node_modules/@ethersproject/web/lib/index.js:14:53)
    at fulfilled (/Users/john/one-wallet/horizon/node_modules/@ethersproject/web/lib/index.js:5:58)
    at processTicksAndRejections (internal/process/task_queues.js:95:5)
(Use `node --trace-warnings ...` to show where the warning was created)
(node:62043) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 1)
(node:62043) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

Thumbs.db

# Ignore built ts files
dist/**/*
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will ignore eth2hmy-relay/ethash/dist/**

@@ -0,0 +1,71 @@
{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can just remove this

@@ -1,2 +1,294 @@
# Horizon Bridge Documentation and API reference

# Overview
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this updated after the other PR (#37)?

@@ -0,0 +1,133 @@
# Advanced Sample Hardhat Project
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove template file? Doesn't seem to be very relevant

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If some commands are helpful - we can keep them in a short cheatsheet

@polymorpher
Copy link
Collaborator

yarn-error.log should be ignored

@polymorpher
Copy link
Collaborator

Can the auto-generated d.ts files be removed? They are more distracting than helpful

@polymorpher
Copy link
Collaborator

Can all auto-generated .md files be removed until we manually review and polish them (e.g. by extensively commenting on original .sol file, or adding extra to .md)? The reason is they don't contain useful information, and may discourage a new developer into thinking all our documentations are just boilerplates

@polymorpher
Copy link
Collaborator

Can data/abi/* be removed as well? They don't look like abi and they are not referenced elsewhere

@polymorpher
Copy link
Collaborator

Lastly, can some template files be removed as well? (e.g. the docs/SOLIDITY.md mentioned in in-line comments). Let's focus on information that is strongly relevant to helping other developers understand and onboard the project

@johnwhitton
Copy link
Contributor Author

johnwhitton commented Jun 8, 2022

Merged latest upstream/main still have some action items for testing and deployment which can be progressed separately.

  • confirm linting has an indentation of 4 spaces
  • Fix test EthLightClient.js
  • Fix deploy script 004_deploy_ethereumLightClient.ts (Fixed by removing deplyments/localnet folder and redeploying)
  • Remove scripts directory (util.js -> src/lib and newtest.js -> test)

@gupadhyaya gupadhyaya merged commit 560c96f into harmony-one:main Jun 9, 2022
@johnwhitton johnwhitton mentioned this pull request Jun 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants