Ethers.js: Why does Signer use a promise for the address?

[alcuadrado]

Hi @ricmoo,

I'm starting to work more with the typescript version of ethers and noticed something curious about the abstract Signer class. It has a getAddress method, that returns a Promise<string> and all the classes extending from it (ie: JsonRPCSigner and Wallet) define the address getter.

I suppose that the abstract one only has the promise version because JsonRpcProvider.prototype.getSinger can receive an index, and return a Signer that has no local address.

This can be refactored and this way:

1. Make JsonRpcProvider.prototype.getSinger return Promise<JsonRpcSigner> and fetch the address there if an index is provided.
2. Add the address property to Signer.

I think it can be a positive change because:

1. It's hard to envision a situation where someone uses the rest of the methods of Signer without knowing its address, so fetching it before initializing a JsonRpcSigner doesn't sound bad. I may be missing some use-case here.
2. It's not clear when you can call JsonRpcSigner.prototype.address or not, losing part of the static safety provided by TypeScript. There's a method defined that depending on how you get your JsonRpcSigner may be uncallable.
3. This refactor would give us a common type of JsonRPCSigner and Wallet that can return its address in a synchronous context, which is handy. This situation is exemplified in the following code:

async function main() {

  const provider = new ethers.providers.JsonRpcProvider("http://localhost:8545");

  const remoteAccounts = (await provider.listAccounts()).map(a => provider.getSigner(a));
  const localAccounts = generateAnArrayOfWallets(provider);
  const accounts = [...remoteAccounts, ...localAccounts]; 

  doSomethingSync(accounts);
}

function doSomethingSync(accounts) {
  for (const account of accounts) {
    console.log(`${account.address}`); // This is not typesafe without an union because of .address
  }
}

What do you think?

[naddison36]

I'd prefer to keep getAddress as a Promise. I've got an Azure Key Vault implementation of Signer. To work out the address I have to call the Key Vault API to get the public key and derive the address.

The same would be for anyone using a secure enclave or hardware device to store keys that are not identified by the address.

[ricmoo]

The AbstractSigner is meant to be a lowest common denominator of what is required for a signer to operate, so that other libraries can reliably have an interface to work with. For all the properties on a signer, they can all operate without knowing the address synchronously (i.e. signMessage and sendTransaction).

I have been thinking of removing the prototype.address all together, as I agree it is complicated to convey when it will work and when it won't. It is sometimes useful, but maybe we should just nuke it?

I would say for the vast majority of cases though, you don't need the address to interact with a signer:

// Examples
let signer = jsonRpcProvider.get();
let anotherSigner = jsonRpcProvider.get(1);

signer.sendTransaction({ to: "ricmoo.firefly.eth", value: parseEther('1.0') });

signer.sendMessage("Hello World!");

let contract = new Contract("registrar.firefly.eth", abi, signer);
contract.register("testing")

But more importantly, this change is for far more broad reason. It enables a lot of new types of signers, such as our experimental LedgerSigner, a new MetaMaskSigner and MultisigSigner.

And @naddison36 provides a few other examples too. :)

[alcuadrado]

Thanks for the quick answers! I wasn't aware the other implementations of Signer so I may be missing lots here.

I guess my worries come from not knowing when JsonRpcSigner.prototype.address can be used or not.

[ricmoo]

I've removed the .address from JsonRpcSigner for now, we can always add it back later. Adding it is not backwards-incompatible, but removing it is, so might as well error on the side that gives us the most flexibility.

I've been thinking of adding a .ready() method on JsonRpcSigner, which would mean it was safe to call .address after it has returned at least once... Still just thinking... :)

Closing this for now, but feel free to re-open if you have any further concerns. :)

[paulrberg]

I would say for the vast majority of cases though, you don't need the address to interact with a signer.

There' a major use case where we do need the address of a signer - our testing environments. Suppose you have the following project:

└─ test
    └─ setup.ts
        └─ foo.ts
        └─ bar.ts

I'm getting my array of wallets in setup.ts, like this:

setTimeout(async function () {
  const wallets = (await ethers.getSigners()) as Wallet[];

  shouldBehaveLikeFoo(wallets);
  shouldBehaveLikeBar(wallets);
}, 5000);

Because the address is not available as a synchronous property, I have to always do this in my down-most test files:

export function shouldBehaveLikeFoo(wallets: Wallet[]) {
  beforeEach(async function () {
    const token: Erc20Mintable = this.token;
    await token.mint(await wallets[0].getAddress(), devConstants.amounts.mint);
  });
}

Notice the ugly "await" among the arguments passed to the mint function.

At the very least, it'd be awesome to be able to override the address property, so we can execute the promise only once at setup time:

setTimeout(async function () {
  const wallets = (await ethers.getSigners()) as Wallet[];
  for (const wallet of wallets) {
    wallet.setAddress(await wallet.getAddress());
  }
  ...
}

[ricmoo]

All Signers that can support synchronous address, have that. I think that is only Wallet and VoidSigner, though?

Most Signers actually cannot get the address synchronously. For example, the JsonRpcSigner needs to fetch the list of accounts (eth_accounts) and use it’s index to figure out its address.

Hardware wallets also cannot know their address, without sending a request and receiving a response over their transport.

Also, you shouldn’t be using as Wallet[], since the classes are not compatible. They both sub-class the same super-class, but they each have multiple methods (and static methods) the other doesn’t have...

[ricmoo]

It would make sense to cache the address though, similar to how the new BaseProvider conditionally caches the network.

The problem before is it was hard to explain to people why sometime the .address was null, but after a resolved call to getAddress() it wasn’t anymore.

I’ll put something similar in today.

[paulrberg]

Also, you shouldn’t be using as Wallet[], since the classes are not compatible. They both sub-class the same super-class, but they each have multiple methods (and static methods) the other doesn’t have...

Waffle requires passing an instance of Wallet in many of their functions, like deployContract. How would you use the accounts as wallets if not with the signers() method (in a testing environment)?

The problem before is it was hard to explain to people why sometime the .address was null, but after a resolved call to getAddress() it wasn’t anymore.

Yes, this would certainly cause some confusion. Why not let users set a flag for this? Something like isAddressCacheable.

[ricmoo]

Waffle should certainly not be using Wallet as their low-level Signer as Wallet is a fairly high-level Signer. The Signer sub-class should be what they use, since a Wallet requires a private key, which many situations do not have access to.

They should provide a Provider sub-class, which has loaded all the accounts (likely from a mnemonic?) and allows access to them, perhaps with a getSigner(indexOrAddress): Wallet; method, if they have a need for Wallet. But they really shouldn't require Wallet, as it does not really offer any additional functionality over a Signer, in terms of its API for the purposes of testing.

I'm not a huge fan of adding flags (which are largely bandaid-ish) as it adds complications, reduces class extensibility (i.e. all of a sudden all sub-classes have an extra thing they must handle), and also clutters the API. This is the reason/value for a clean class hierarchy in the first place...

All my test cases (written in Mocha), use just a normal Signer, there is no need for the requirement of a Wallet.

I'll bug them. :)

[paulrberg]

Fantastic, thanks for the thorough explanation.

A lot of examples on the first page of Google that show how to use Waffle and Ethers make a casting from Signer to Wallet, so this issue might be more pernicious than I thought. I'll ping them too!

[paulrberg]

Update for Hardhat

We don't have to handle Promises anymore! As per the Hardhat changelog:

The signers returned by ethers.getSigners() have an address property now. No more await signer.getAddress() everywhere.

Example:

it("my test", async function () {
  const [owner] = await ethers.getSigners();
  const ownerBalance = await token.balanceOf(owner.address);
});