Although it will be a bit verbose, this looks like a good spot for a function overload

@andrewpeters9 what do you think about a solution with type generics?

async #signTransaction<T extends TypedTransaction | OldEthJsTransaction>(
    address: string,
    chainId: number,
    tx: T,
    handleSigning: (tx: EthereumSignedTx) => T,
): Promise<T>

This version of the package has a peerDependencies conflict; it is asking for a different version of eslint than the other config packages are. Generally it's better to keep these in sync with each other so that they all work more predictably.

To bring this in-line with the other configs, you'd want to change this to ^8.0.0 and downgrade the @typescript-eslint packages to ^4.20.0. We can update the configs together in a separate PR.

Hmm. I see that this problem goes deeper. The version of ESLint we're using isn't compatible with any TypeScript version beyond v4.4.4, so we'd need to downgrade TypeScript as well, and that would force us to downgrade typedoc.

Maybe we can leave this as-is and update the ESLint configs to resolve this peer dependency conflict in a later PR.

Huh, interesting. This cast seems to be necessary because tx.toJSON claims that a bunch of things are potentially undefined even though they aren't. I looked at the source for toJSON, and this is what it returns:

 {
  nonce: string;
  gasPrice: string;
  gasLimit: string;
  to?: string;
  value: string;
  data: string;
  v?: string;
  r?: string;
  s?: string;
}

I tried casting the return value of toJSON to that value directly, and then the cast on this line wasn't needed. So what we're doing here should be type-safe, if the return value of toJSON was correct.

Forgive me if this has been asked already, but is there any way that we can avoid this typecasting?

@mcmire TS complains that the object might be instantiated with a different subset of MessageTypes, as we destructure dataWithHashes.types to give default values.
We can maybe avoid the cast and assign default values anyway in this way:

const {
      types,
      message = {},
      domain = {},
      primaryType,
      domain_separator_hash,
      message_hash,
    } = dataWithHashes;
    
// ...

data: {
    types: { ...types, EIP712Domain: types.EIP712Domain ?? [] },

What do you think?

Yeah I think that would make sense!

Hmm, we can use extends here for tersity, but it's technically incorrect (unless we want to be allowing input that's an extension of those objects?) And we would be returning a less narrow type.

The way to overload this would be:

  async #signTransaction(
    address: string,
    chainId: number,
    tx: OldEthJsTransaction,
    handleSigning: (
      tx: EthereumSignedTx
    ) => (OldEthJsTransaction )
  ): Promise<OldEthJsTransaction>;
  async #signTransaction(
    address: string,
    chainId: number,
    tx: TypedTransaction,
    handleSigning: (
      tx: EthereumSignedTx,
    ) => (TypedTransaction)
  ): Promise<TypedTransaction>;
  async #signTransaction(
    address: string,
    chainId: number,
    tx: OldEthJsTransaction | TypedTransaction,
    handleSigning: (
      tx: EthereumSignedTx,
    ) => (OldEthJsTransaction | TypedTransaction)
  ): Promise<OldEthJsTransaction | TypedTransaction> {

but it's technically incorrect (unless we want to be allowing input that's an extension of those objects?

Hmm, it sounds like you're misunderstanding this keyword. It really doesn't imply any sort of OOP-style inheritance. Nothing about using extends with a type union is incorrect. This is a conventional and effective way to ensure that a generic parameter is one of two types.

This is a conventional and effective way to ensure that a generic parameter is one of two types.

Yes, but it allows for a different type that happens to extend OldEthJsTransaction | TypedTransaction to be passed in too - which I don't think we want to allow. E.g.:

type Tx1 = {
    name1: string;
}

type Tx2 = {
    name2: number;
}

const test = <A extends Tx1 | Tx2>(a:A, fn: (x: any) => A): A => {
    return a;
}

test({name1: "test", extraKey: "test"}, (x) => ({name1: "test", extraKey: "test"})); // ✅
test({name1: "test", name2: "test"}, (x) => ({name1: "test", name2: "test"})); // ✅

Whereas if we use overloads:

type Tx1 = {
    name1: string;
}

type Tx2 = {
    name2: number;
}

function test (a: Tx1, fn: (x: any) => Tx1): Tx1;
function test (a: Tx2, fn: (x: any) => Tx2): Tx2;
function test (a: any, fn: (x: any) => any): any {
    // last overload is ignored, so we can use 'any' for the type
    return a;
}

test({name1: "test", extraKey: "test"}, (x) => ({name1: "test", extraKey: "test"})); // ❌
test({name1: "test", name2: "test"}, (x) => ({name1: "test", name2: "test"})); // ❌
test({name1: "test"}, (x) => ({name1: "test"})); // ✅
test({name2: 4}, (x) => ({name2: 4})); // ✅

Ahh I see. Interesting, good point.

Using any in the implementation does effectively disable type checking though, which isn't ideal. We should at least use the type union there, getting us at least as much type safety as we had before.

Thinking about this further though, this seems based on the false assumption that we're expecting exact transaction objects here. The keyring interface we're using doesn't demand that, it allows additional properties. That is, the old type union seems more-correct even with the surprising behavior you've demonstrated in this code snippet.

Using any in the implementation does effectively disable type checking though

When using overloads, the last type definition is ignored completely, so any doesn't disable type-checking here

Regarding extended types - ok good point. I'll take a brief look into this today

@Gudahtt I had a look into the code today. Given that the tx objects are being created within the keyring itself, and not publically, is there a scenario where we would want to allow the extension of the types?

I've found another benefit to using overloads here, as old transactions should be passing through a Buffer instead of a number. (See my comment on the overload)

@andrewpeters9 see my comment regarding the chainId coming from the old tx.
Moreover, recently a Keyring type has been added to utils (See https://github.com/MetaMask/utils/blob/main/src/keyring.ts#L137) so we should refactor this package to use that interface in a separate PR, which uses just the TypedTransaction - this should simplify a lot the implementation too as we will not expect an old tx anymore.

Do you think we could maybe track the object extensibility problem you are highlighting in a separate issue?

Given that the tx objects are being created within the keyring itself, and not publically

They're currently being created in the transaction controllers (in the core repo for mobile, and in metamask-extension for the extension).

@Gudahtt I had a look into the code today. Given that the tx objects are being created within the keyring itself, and not publically, is there a scenario where we would want to allow the extension of the types?

I've found another benefit to using overloads here, as old transactions should be passing through a Buffer instead of a number. (See my comment on the overload)

Nit: this could be readonly too

Potentially out of scope for this PR, but TrezorConnect.dispose() returns a promise, so making this promise access to outside the class might be handy. E.g.:

   dispose(): Promise<void> {
       return TrezorConnect.dipose():
    }

Good catch! Doing it in a subsequent PR would probably be best. Tracked here: #170

We can remove as unknown as number and let the type pass through as a Buffer - see overload comment.

we can change this to chainId: Buffer,

@Gudahtt This is the other benefit I was mentioning regarding overloads. The chainIds for old tx's have to be Buffers instead of numbers

@andrewpeters9 that getChainId() method is documented to return a Buffer, but it actually will return always a Number (see https://github.com/ethereumjs/ethereumjs-tx/blob/v1.3.7/index.js#L126).

This cast should be safe and we could leave this #signTransaction method as-is.

This would then have to become chainId: any,

I just checked, maybe my TS engine is playing up, but it looks like we can remove as EthereumTransaction | EthereumTransactionEIP1559 here too

Nit: but if you want to ignore IDE generated files you can add these lines (these are included in metamask-extension too):

# IDEs
.idea
.sublime-project