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

Refactor ABI Gen package #2877

Closed
arboleya opened this issue Aug 1, 2024 · 2 comments
Closed

Refactor ABI Gen package #2877

arboleya opened this issue Aug 1, 2024 · 2 comments
Assignees
Labels
feat Issue is a feature

Comments

@arboleya
Copy link
Member

arboleya commented Aug 1, 2024

TBD

@arboleya arboleya added epic feat Issue is a feature and removed epic labels Aug 1, 2024
@arboleya arboleya added the epic label Aug 1, 2024
@arboleya arboleya added this to the Caterpillar v1 milestone Aug 2, 2024
@nedsalk
Copy link
Contributor

nedsalk commented Aug 20, 2024

There are several goals of the refactor:

  1. Merge the abi-coder and abi-typegen packages into one. Merge abi-coder and abi-typegen packages #2346
  2. Create an abstraction on top of JsonAbi which abi-coder and abi-typegen can build on, so that changes to the abi specification are localized to that abstraction while coders and typegen remain unaffected.
  3. Possibly have the approach to interpreting that abstraction be the same in coder and typegen and going from abstraction to coder/typer be the same
  4. Refactor typegen "typers" (IType interface implementers)
  5. Figure out a better coder design. Refactor the ABI Coder package #1795

Items 1-4 can be done immediately because they're internal details, whereas item 5 must be left for v1 because all the coders are currently exported to the users.

Pre-v1

1. Merge the abi-coder and abi-typegen packages into one.

This is a straightforward task and should just require putting the two packages together into a single abi package (coder and typegen folders) and removing the duplicate code: JsonAbi interfaces and the abi transpiler.

2. Create an abstraction on top of JsonAbi

This was already done in #2826 with the ResolvableType and ResolvedType abstractions. The goal of this item would be to update the folder structure of the abi package src now has three folders: parser, coder and typegen. The abstraction would go into the parser folder. The coder and typegen implementations would then reference the abstraction in appropriate places. Inspiration can be taken from the referenced PR.

3. Interpret the abstraction the same way in typegen and coder

This is related to how we go from the abstraction to a specific coder/typer. The coder does this via the getCoder function, while typegen does it via the parseType function. The difference between them is that suitability of a coder for a specific type is determined in the getCoder function directly, whereas in typegen each type has an isSuitableFor(type) static method which checks for suitability of that specific type. Both approaches are okay and easy to digest and working on this gives the least bang for the buck out of all the items.

4. Refactor typegen "typers"

After working with abi-typegen extensively in #2826 I realized that the IType interface and its implementers can be simplified after introduction of the abstraction from item 2. The changes can be seen in the referenced PR, but they boil down to immediately resolving input and output labels upon instantiation, having only fields on IType and no methods that have to be called for additional post-processing. Please note that the referenced PR's implementation falls short from this in some aspects and should be improved upon, but is a step in that direction.

We should also reevaluate the need for some tests in typegen at this point because they'll start breaking once the refactoring commences. I believe that it is enough for us to have the full sway project be the exhaustive source for verifying input and output label correctness of all types via this test and that we don't need tests for each "typer" individually.

v1

5. Figure out a better coder design

Info on this can be found in #1795.

@arboleya
Copy link
Member Author

arboleya commented Sep 7, 2024

Closing in favor of:

@arboleya arboleya closed this as not planned Won't fix, can't repro, duplicate, stale Sep 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat Issue is a feature
Projects
None yet
Development

No branches or pull requests

3 participants