Remove (magic) deferred input types

[fubhy]

Hi!

Interfacing with ethereum leads to a few tough challenges. One of the biggest ones arguably being parameter conversions and formatting. Currently, ethers.js goes to great lengths to try and (asynchronously) resolve arbitrary input parameters for contract functions to their respective parameter types. I am assuming that this was done to try and improve the developer experience by being less restrictive when passing parameters to contract functions. While the intent is commendable, the concrete implementation leads to a plethora of problems that arguably negate the superficial DX improvement. Ambiguous parameter types and type conversions and passing of deferred values lead to impossible-to-debug scenarios as well as gridlocks of inter-dependent promises and undesired side-effects (e.g. ENS address resolution).

Here are just a few of the more concrete problems that arise from this:

Handling of input parameters for contract calls

1. It's currently possible (and I've personally run into it) to end up in a never resolving promise where two values depend on each other, therefore try to resolve each other but due to them depending on each other never do.

Example: Deploying two contracts that allow each other to be registered through their respective constructor arguments. Since it's allowed by the API to pass each of the contract instances as arguments to each other, it's possible to get stuck in a gridlock. Now, if both of these deployments happen side-by-side in the same file it's easy to find the culprit, but in a more complex deployment script it can be quite tricky to find the reason for the script getting stuck.

2. Passing promises into an asynchronous function means that errors handling is obscured by the internals of this function. This leads to really hard-to-debug situations and undesired side-effects.

Imagine you accidentally passed a wrong value (promise of an arbitrary string) to a function parameter of ParamType "address", ethers.js will now try to resolve this promise for you, realizes it's not a valid address and then go through a range of different attempts to resolve this address. Once it fails to resolve it, the error the user is presented with comes from deep down in the function call stack of ethers.js and it's impossible to find the actual culprit (the line of code where you are accidentally passing the wrong parameter). In summary: Functions should never resolve promises on behalf of the callee unless that's their only purpose (so you can await it so that the error bubbles up all the way to the callee in a predictable way).

3. An automatic fallback that attempts to resolve non-address values through ENS means that any invalid address value is then forwarded to ENS. There is no clear signaling of intent by the callee, and thus any error where the user accidentally passed a wrong, non-address value to the function is obscured and therefore hard to debug.

Attempting to automatically resolve strings through ENS when passed to an address ParamType without clear intent communicated by the callee could lead to potential dire consequences. The scenario is quite unlikely but it's possible that you end up calling your function with a wrong value without even realizing what's going on. Furthermore, if the resolution of the address fails, the error is again coming form deep down in the ethers.js call stack making it extremely hard to debug. I've seen people complain about this in the buidler social media channels quite regularly over the past weeks ("Why is Buidler trying to call ENS?"). TLDR: A function should not attempt to arbitrarily upcast values through an asynchronous service that yields unpredictable results unless the callee is explicitly signalling intent to do so (by explicitly calling and awaiting a function for that)

Suggestion for enforcing strict TypeScript scalar types (including runtime validation)

TypeScript actually has quite a bit more to offer than just it's automatic type inference and static type definitions. One of the features that could prove to be quite interesting and helpful for this particular problem space (interfacing with the strict type universe of solidity) are so called "custom" or "user defined" type guards (built-in typeguards are e.g. typeof or instanceof and derived checks). Together with unique symbols, it would be possible to create a solidity type dictionary within ethers.js that could then be enforced in user space, resulting in a few great benefits (despite the arguably increased syntax verbosity).

1. The types enforced by the ABI would be immediately visible in the TypeScript source code too.
2. No deferred or otherwise obscured errors deep down in the ethers.js call stack
3. No function overload ambiguity (once types are strictly enforced, it becomes super easy to map a function signature to the concrete overload simply by matching the function signature against the passed types)
4. Code confidence

Let me illustrate that through an example using e.g. the address scalar:

import { utils } from 'ethers';

// Declare a uniquely identifiable type for addresses based on `string`.
export type address = string & {
  readonly __: unique symbol;
};

// Type guard for the `address` type.
export function isAddress(value: any): value is address {
  return typeof value === 'string' && !!value.match(/^(0x)?[0-9a-fA-F]{40}$/);
}

// Run-time validator for the `address` using the type guard for strong type inference.
export function address(value: any): address {
  if (!isAddress(value)) {
    throw new Error(`Invalid address value: ${value}`);
  }

  // Convert to the checksum to allow simple string-to-string comparison.
  return utils.getAddress(value);
}

// myContractCall(address _input) view returns (uint256 output_)
function myContractCall(_input: address): Promise<uint256> {
  // Do something.
}

myContractCall('0x123123123'); // not valid in typescript
myContractCall(address('0x123123123')); // valid in typescript but throws at runtime

const weth = address('0xc02aaa39b223fe8d0a0e5c4f27ead9083c756cc2');
myContractCall(weth); // valid in typescript and also at runtime

Note how this could also be used to strictly type the output of such a call. In this case, uint256 would simply be a constrained type wrapper around BigNumber.

Any incorrectly passed parameters or otherwise wrong values passed to such a function would immediately fail in user-space code and be super easy to pinpoint and debug because due to TypeScript requiring the usage of the type guard, the user would have no choice other than call address() on the passed argument. Hence, we'd be covering both development-time and runtime errors.

[fubhy]

Dang, now I wish I'd waited for 3 more issues to be filed before mine just to be #1000 :(

[ricmoo]

Dang, now I wish I'd waited for 3 more issues to be filed before mine just to be #1000 :(

LOL! Better luck next time. ;)

I personally use ethers a lot... Like, a lot-lot. Which is why I wrote and maintain it in the first place. Like, I can't imagine someone using it more than me. :p

Which is one of the reasons I use (optionally) deferred types, which is a feature of ethers I use extensively, both as a consumer of the library and internally. It also allows the library to be "more optimal" in many cases, since otherwise I see people performing multiple awaits, not realizing they just massively serialized their execution.

Deferred types are only used when the entry point itself is a something that is asynchronous; for example, provider.getBalance, which already returns a promise, so any extra promise resolving that goes on can be handled transparently. This is also how ENS as a first-class citizen works; passing in an ENS name can just get magically wrapped as a provider.resolveName.

I won't argue that async things are often difficult to debug, but things are often difficult to debug (in general), so I don't think that is a good enough reason to remove something I consider so useful. You can of course pass in only resolved properties, in which case many of these (but I agree, not all) go away.

3. No function overload ambiguity (once types are strictly enforced, it becomes super easy to map a function signature to the concrete overload simply by matching the function signature against the passed types)

I've thought of this before as well, and am considering it for v6. The syntax overhead goes through the roof, but for people who want to use it, it might make sense. Keep in mind that Deferred types could still work fine, as a uint48 (or smaller) could accept a number | Promise<number>. The main issue is more the additional syntax (and the number of additional names in the namespace); since these seem similar in complexity: contract["foo(uint48)"](v) vs contract.foo(uint48(v)). But it is a possibility. I need to try it out first.

Which brings up a quick point, I have tried both deferred and non-deferred solutions; I often (when making the beta major changes) make changes like this, and port an application or few over to use the new method to determine how it "feels".

In v6, the Deferrable type will be more prevalent, but I didn't want to change the signatures in v5 at the last minute when I started using it.

Anyways, I don't know that I've appeased you at all, but I am highly unlikely to removed the deferrable types; I am working on making error messaging more clear as to why/where things failed though.

I'll leave this around though, in case you or anyone else wants to continue the discussion, but I'll move it to the new discussions features. :)

Thanks! :)

(sorry for the delay, but I knew this would be a longer-than-usualy-resposne, and I still didn't get to all the points that went through my head, but it's probably better to get something out, than leave this around seemingly abandoned. ;))