Implement partial transpilation #130

Amxx · 2023-09-19T13:52:34Z

No description provided.

Amxx · 2023-09-20T17:55:52Z

src/transformations/fix-import-directives.ts

-        throw new Error(`Can't find symbol imported in ${ast.absolutePath}`);
+declare module '../transform' {
+  interface TransformData {
+    importPath: string;


I'm open to a better name for this.

Meaning is:

for nodes that are contract definition (includes libraries and interfaces)

should the node not be transpiled

if so, where should it be imported from.

said otherwize:

undefined means object should be transpiled as expected (no particular treadment)

string means the object should not be transpiled

code should be removed

imports in other files should like to this importPath

reference to this object should not be renamed

I'm also wondering if this is the right location to declare that field.

Could be importFromPeer?: string.

Note that it should be defined as an optional.

In fix-new-statement.ts, isUsedInNewStatement is not optional

getData has type (node: Node) => Partial<TransformData>;

So why would it be marked optional here ?

src/cli.ts

src/transformations/fix-import-directives.ts

src/transformations/remove-partial.ts

src/transformations/rename-identifiers.ts

src/transform.ts

src/index.ts

frangio

Looks pretty good but I left some important comments below.

One thing that I see in the spec and not addressed here is throwing an error if an excluded definition contains a reference to a transpiled definition. Is that something we want to do? I think one example of this would be: suppose SafeERC20 had references to ERC20 instead of IERC20. Should that be a transpiler error? Now that I think about it it doesn't sound like that would be a problem in any way.

src/transformations/fix-import-directives.ts

src/index.ts

frangio · 2023-09-25T00:25:56Z

src/index.ts

+    for (const node of findAll('ContractDefinition', ast)) {
+      if (node.contractKind === 'contract') {


In our spec we had written that we'd also exclude "contracts with @custom:need-not-transpile".

Do we want to do this? Do we need it for OZ Contracts?

If we do it, we will need a new helper similar to extractContractStorageSize in utils/natspec.ts.

Do we need it for OZ Contracts?

Not really. Maybe that could replace some of the "initializable" specific logic, but its not something we absolutelly need.
If we do so, do we also add a @custom:force-transpile tag that does the opposit ?

@custom:force-transpile is not what we need, I can't imagine a use case for it. I think the only contract where need-not-transpile could be relevant is UUPSUpgradeable. The current logic transpiles UUPSUpgradeable, though for this contract in particular we have a trick which is that we don't add the *Upgradeable suffix because the identifier already has it. Since it doesn't have any storage variables it won't get a namespace either. It does have initializers added.

Perhaps it's best to keep transpiling this contract and others simply for consistency, the initializers might be enough of a reason.

Should "@Custom:need-not-transpile" apply to contracts or also to other objects (free function, structure definition, ...)

In the context of "peerImport", they would automatically be skiped, but we could imagine a project without "peerImport", where everything is transpiled by default, except for some @custom:need-not-transpile objects.

Also, what if they are used anywhere, and we don't have a peerImport path for them? How should we deal with that? Throw an error?

Also, what if they are used anywhere, and we don't have a peerImport path for them? How should we deal with that? Throw an error?

Yeah that's a good question.

If we don't need this for OZ Contracts, let's not do it.

src/transformations/remove-partial.ts

src/transform.ts

Co-authored-by: Francisco <[email protected]>

Amxx · 2023-09-25T10:32:48Z

In the current implementation, if a peerProject is set:

contracts are transpiled
anything else, declared at the file level, is NOT transpiled. This includes :
- interfaces: I think these should not be transpiled
- libraries: This should not be transpiled, except maybe functions that take contract (that are transpiled) as arguments.
- custom errors: I think these should not be transpiled
- free functions: Same as functions in libraries. We only need to transpile them if they take arguments that are contract.
- UDVT: I think these should not be transpiled
- structs: Same as libraries and free function. It may contain references to contracts that are transpiled.

Are these assumptions reasonables?

Functions in libraries don't have contract arguments, and use interfaces instead.
Free functions don't have contract arguments, and use interfaces instead.
Both of the above do not call functions that take contract arguments
- I tink this is ok, because in order to call such functions, they would either need to get these as arguments, or do a casting.

If the assumptions are ok, do we want to check them?

frangio · 2023-09-25T13:42:45Z

Are these assumptions reasonables?

I think they are reasonable, but moreover if they are broken I don't think it would cause any correctness/security issues, only usability issues. If you agree with that, I don't think we need to validate the assumptions.

src/transformations/peer-import.ts

Co-authored-by: Francisco <[email protected]>

frangio

LGTM!

Implement partial transpilation

c4ac4cc

Amxx requested a review from frangio September 19, 2023 13:52

Amxx added 11 commits September 19, 2023 16:14

missing newlines

06f00f1

use getData to mark object that are not transpiled

8064ec1

remove non-transpiled objects from files that are transpiled

890d8de

fix

48d3f61

lint

71e16a9

remove unecessary eslint-disable

f6cd41a

declare TransformData.importPath in the fix-import-directives file

4b19e44

fix bug when a.local === undefined

bfcb1e1

refactor import for Initializable

9b831ea

update test cache

46c62ab

change linting of the produced solidity code

d216e47