-
Notifications
You must be signed in to change notification settings - Fork 16
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
Implement partial transpilation #130
Conversation
throw new Error(`Can't find symbol imported in ${ast.absolutePath}`); | ||
declare module '../transform' { | ||
interface TransformData { | ||
importPath: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be importFromPeer?: string
.
Note that it should be defined as an optional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- In
fix-new-statement.ts
,isUsedInNewStatement
is not optional getData
has type(node: Node) => Partial<TransformData>;
So why would it be marked optional here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/index.ts
Outdated
for (const node of findAll('ContractDefinition', ast)) { | ||
if (node.contractKind === 'contract') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Co-authored-by: Francisco <fg@frang.io>
In the current implementation, if a peerProject is set:
Are these assumptions reasonables?
If the assumptions are ok, do we want to check them? |
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. |
Co-authored-by: Francisco <fg@frang.io>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
No description provided.