Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move AST to standalone tiny package #124

Open
blitz-1306 opened this issue May 25, 2022 · 3 comments
Open

Move AST to standalone tiny package #124

blitz-1306 opened this issue May 25, 2022 · 3 comments
Labels
breaking change Changes that would cause a backward compatibility break

Comments

@blitz-1306
Copy link
Contributor

blitz-1306 commented May 25, 2022

Current package has two goals:

  • Handle compile routines of Solidity sources. This requires package to rely on Solc compiler, do file system lookups, allow compiler guessing and downloading.
  • Handle AST processing. This requires to handle historically various types of Solidity AST (produced by compiler), tree travesal, AST-to-source writing, making new AST nodes, also parse types to a type tree.

We are considering to move AST processing to a standalone tiny package in near future. Current reasoning:

  • Compile-related functionality forces to load ~20 megabytes by always shipping and loading latest WASM compiler.
  • Compile-related functionality requires to do a file system lookups. This is useful when using sol-ast-compile or developing side CLI packages, but not that suitable in case if someone would like to use typed AST in browser.
  • AST processing actually does not force to use our compiling routines. In other words, ASTReader does not care how source were compiled - it requires only compiler output. Users may use native, wasm or whatever compiler to compile source and just supply returned data.

A plan:

  • Move AST-related functionality to a side package and publish it to NPM.
  • Clean up this package from AST logic.
  • Add AST package as dependency to maintain BC of this package however possible.

We are publising this to receive opinions and infrom about further changes. Thanks for your time and attention.

@blitz-1306 blitz-1306 added the breaking change Changes that would cause a backward compatibility break label May 25, 2022
@blitz-1306
Copy link
Contributor Author

blitz-1306 commented Jul 19, 2022

Current set of considerations:

  • both: update README in both repositories.
  • both: publish to NPM as two separate packages.
  • both: release cycle would be longer, need to update/release ast and compile packages and have common flow for release procedure.
  • both: need to figure out what to do with the docs.
  • ast: Properly verify (or consider to drop) sanity checking logic #117.
  • ast: should not have a docker image.
  • ast: would miss integration tests, so there would be necessary to figure something (maybe use WASM-produced and native-produced artifacts).
  • compile: should use ast as dependency.
  • compile: should contain compile-related logic and executable (sol-ast compile)
  • compile: should have integration tests.
  • compile: ammouncement in README that packages are now separated.
  • compile: probably reexport ast to maintain backward compatibility.

@blitz-1306
Copy link
Contributor Author

blitz-1306 commented Jul 19, 2022

@cd1m0 Hello. Consider some of the steps above if there would be some spare time. Thanks.

@cd1m0
Copy link
Contributor

cd1m0 commented Jul 22, 2022

These are all valid considerations. I just want to comment on two of them:

  1. "release cycle would be longer": you are right, this is a downside of this approach. I don't know of a way to avoid this

  2. "ast would miss integration tests": yeah, this will be a problem. One thing we could try is to add the compilers and file-system packages as dev dependencies, and copy over a minimal set of logic for compiling contracts. Then we might still be able to adapt some of the integration tests from the compile repo?

Everything else in the considerations seems ok to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Changes that would cause a backward compatibility break
Projects
None yet
Development

No branches or pull requests

2 participants