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

Remove method overloads #61

Open
dtbuchholz opened this issue Jan 27, 2025 · 3 comments
Open

Remove method overloads #61

dtbuchholz opened this issue Jan 27, 2025 · 3 comments

Comments

@dtbuchholz
Copy link
Contributor

dtbuchholz commented Jan 27, 2025

This isn't necessarily essential, but viem tends to struggle with heavily overloaded methods and type inference. We can either:

  • Keep things as-is, and better understand if there are ways to resolve this easily with viem (it's possible the JS SDK didn't manage this optimally)
  • Remove overloads from only the ABI (manually or scripts) when it's used in external libs, e.g., the JS SDK or Studio
  • Remove the overloads entirely so that every method has a single function signature

Overloads were created to help facilitate a better DX for Solidity-first developers. However, the only user for the foreseeable future is internal client tooling (JS SDK, Studio, etc.).

@dtbuchholz
Copy link
Contributor Author

cc @asutula idk if you've ran into any issues here yet wrt the Studio. some methods (like querying objects) have a handful of overloads. when I built out the SDK, I manually removed some ABI methods so that only one method was included in the ABI (i.e., the method that had "all" of the params). this is because I statically defined the types (like this), and viem doesn't handle overloads well, so I ran into type issues with the TS compiler.

just a heads up in case you run into the same problem.

@asutula
Copy link
Contributor

asutula commented Feb 3, 2025

cc @asutula idk if you've ran into any issues here yet wrt the Studio. some methods (like querying objects) have a handful of overloads. when I built out the SDK, I manually removed some ABI methods so that only one method was included in the ABI (i.e., the method that had "all" of the params). this is because I statically defined the types (like this), and viem doesn't handle overloads well, so I ran into type issues with the TS compiler.

just a heads up in case you run into the same problem.

I did run into similar issues, but basically just have to be very careful and explicit about how I construct args and pass them to the function calls. After struggling a while with it, I got the hang of it. It seems like the overloads are good to have just for gas savings. But if that's not so important, I'm also fine with removing them.

The thing that was more confusing to me is that it seemed like in some places it was a bit arbitrary which overloads existed and which didn't.

You mentioned maybe the work on the facades would clean up this situation... Is that true? Should we wait and see?

@dtbuchholz
Copy link
Contributor Author

The thing that was more confusing to me is that it seemed like in some places it was a bit arbitrary which overloads existed and which didn't.

yeahhh, I stopped creating them as time went on due to the viem issues and the complexity of params (too many different ways to overload). it seemed like it'd be easier to just handle overlaods at the client layer (if you're aware of the "C" like behavior wrt Rust Option / null types.)

You mentioned maybe the work on the facades would clean up this situation... Is that true? Should we wait and see?

um, I think it'll just make it easier to understand by way of better documented behavior. i guess that doesn't really depend on the facades though....i could improve the docs of the existing codebase to make it less opaque. however, i'd imagine we wont use any overloads with the facades. it's not that we cant include overloads there, but it'll probably be easier to have a 1:1 facade method mapped to each rust method.

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

No branches or pull requests

2 participants