Extension refactoring ideas

[memasdeligeorgakis]

This collects some notes and refactoring ideas for the extension.

[memasdeligeorgakis]

/namada-interface/packages/storage/src/store/MemoryKVStoreProvider.ts is named by a class in it, but not the exported class. I think it should be MemoryKVStore.ts.

[memasdeligeorgakis]

namada-interface/packages/storage/src/store/index.ts seem to be exporting export * from "./BaseKVStore"; but it seem not to be used outside of this module.

[memasdeligeorgakis]

Add comments to explain the meaning of the key types, such as

export enum KeyStoreType {
  Mnemonic = "mnemonic",
  PrivateKey = "private-key",
}

export type DerivedAccount = {
  id: string;
  address: string;
  alias?: string;
  establishedAddress?: string;
  parentId?: string;
  path: Bip44Path;
  type: KeyStoreType;
};

[memasdeligeorgakis]

Add comments & tests

// namada-interface/apps/extension/src/App/Accounts/AddAccount.tsx

const validateAccount = (
  account: number,
  newAccount: { change: number; index: number },
  accounts: DerivedAccount[]
): boolean => {}

// and

const findNextIndex = (accounts: DerivedAccount[]): number => {}

[jurevans]

/namada-interface/packages/storage/src/store/MemoryKVStoreProvider.ts is named by a class in it, but not the exported class. I think it should be MemoryKVStore.ts.

FYI - This has been deleted entirely in the next PR: #105, along with BaseKVStore

[jurevans]

namada-interface/packages/storage/src/store/index.ts seem to be exporting export * from "./BaseKVStore"; but it seem not to be used outside of this module.

This file has been removed in #105

[memasdeligeorgakis]

In namada-interface/packages/types, we are using types from @keplr-wallet/types. However @keplr-wallet/types is not defined as a dependency in namada-interface/packages/types but in namada-interface/packages/integrations as `

//namada-interface/packages/integrations/package.json
"dependencies": {
  "@keplr-wallet/types": "^0.10.19"
}

1. Do we want to become dependent on the types of other projects (and not just encapsulate the usage of Keplr in our app in some wrapper)? 1.1 If we do it, then likely we would like to pin the dependency to a specific version.
2. We should define the dependency in all the packages that are using them. So here likely import from that package that defines Keplr if we are to use it.

[memasdeligeorgakis]

There is a jumpy UI element. It happens as it gets a border of 1px only when pressed down.

https://user-images.githubusercontent.com/2773320/196744707-3fced2f0-9177-4768-b3c5-39db58745b00.mov

[memasdeligeorgakis]

Change name to plural

// namada-interface/apps/extension/src/router/Router.ts

export abstract class Router {
  protected msgRegistry: MessageRegistry = new MessageRegistry();
  protected registeredHandler: Map<string, Handler> = new Map();

// to 

export abstract class Router {
  protected msgRegistry: MessageRegistry = new MessageRegistry();
  protected registeredHandlers: Map<string, Handler> = new Map();

[memasdeligeorgakis]

// apps/extension/src/App/Accounts/AddAccount.tsx

const validateAccount = (
  account: number,

to

// apps/extension/src/App/Accounts/AddAccount.tsx

// this validates for now that the account do not exist before with the given path
const validateAccountDoNotExist = (
  account: number,

[memasdeligeorgakis]

I feel that it is more effortless to defined and maintain types for Borsh serializing/deserializing in Rust and just expose all needed types/funcs from there. We have some at namada-interface/packages/types/src/tx/schemashielded.ts and a few others in the containing folder.

In case where some functionality is on the TypeScript side, unified naming (class and it's schema definition) and keeping only the ones required outside of file public.

[memasdeligeorgakis]

refactoring idea

// apps/extension/src/provider/Signer.ts

import {
  TransferMsgValue,
  TransferMsgSchema
} from "@anoma/types";

const transferMsgValue = new TransferMsgValue({
  source,
  target,
  token,
  amount,
  key,
  shielded,
});

const transferMessage = new Message<TransferMsgValue>();
const serializedTransfer = transferMessage.encode(
  TransferMsgSchema,
  transferMsgValue
);

to

// apps/extension/src/provider/Signer.ts

import {
  TransferMsgValue,
} from "@anoma/types";

const transferMsgValue = new TransferMsgValue({
  source,
  target,
  token,
  amount,
  key,
  shielded,
});

const borshEncodedTransferMessage = transferMsgValue.toBorshEncoded();

[memasdeligeorgakis]

Idea for better type safety and easier refactoring: In apps/namada-interface/src/slices/transfers.ts submitTransferTransaction we get an integration and use signer from that. We get it by using a getter and then casing it to Signer. Further, that integration is of type Integration (apps/namada-interface/src/services/integrations.tsx).

Integration is type Integration = typeof Anoma | typeof Keplr | typeof Metamask;

All of these implement Integration so they contain integration(). But they all return a different thing from signer: signer () => S | undefined. Only Namada returns Signer so if there is another "Integration" returned by the signer(), type system does it but it will crash.

Or maybe a type predicate for signer could be used to at least handle the case where possibly try to cast the wrong type.

[memasdeligeorgakis]

An idea for establishing more clear connection between types where Borsh schema is defined in TypeScript. I feel that it could lead to annoying debugging sessions, when Rust suddenly cannot deserialise something:

In the current state we pass some objects from TypeScript to Rust using borsh-js. In these cases we have defined a schema in TS and using that schema we turn objects to Uint8Array and pass them to Rust functions. On the Rust side the byte array is again deserialised to Rust objects. This works but we have to do the mapping based on mere strings and not any actual types that would warn us when we refactor or simply change the types.

Starting to do some inline types in JS might also not be good, as it might need some hacks and tricks. Maybe simply placing a URL to the schema definition next to the Rust types that we do like this.

Otherwise some Serde deserialization might be more robust to bring the objects over.