Online signer Prototype

[ethanfrey]

Closes #316

This is my API design for OnlineSigner and a default version that wraps OfflineSigner. Discussed with @willclarktech and @webmaster128 and generally agreed as a decent design for an OnlineSigner that presents a UI.

TODO:

• Add SignedTx to the return value of OnlineSigner.signAndBroadcast
• Move fee lookup into InProcessOnlineSigner and not in SigningClient.

[willclarktech]

So as I understand it, there are two advantages of using an online signer over the offline signer we've already implemented:

1. It allows the signer to override/set properties of a transaction which were set/left blank by the application requesting a signature (in particular fees/gas).
2. It allows the signer itself to control the broadcasting of the transaction, which prevents a potential class of mischievous app behaviour involving asking for signatures and then using the signed in a way the user wasn't expecting. E.g. telling the user the transaction failed somehow, waiting for a user action reflecting that fact, and then successfully broadcasting the transaction later.

Are we clear on how realistic these advantages are? E.g. is it plausible to support a decent UX for point 1?

[webmaster128]

It allows the signer to override/set properties of a transaction which were set/left blank by the application requesting a signature (in particular fees/gas).

Good point.

So far I thought the desired feature is the ability to override the fields as a human. Do we even want to allow leaving them empty? Is an online signer able to determine the gas limit for all message types on all chains it connects to?

[webmaster128]

This is a good start and nice way to combine the two interfaces. Here is some initial feedback on this PR:

1. signAndSubmit sould be renamed to signAndBroadcast (see also Rename "PostTx" to "BroadcastTx" #317)
2. Decide who sets sequence, account number and fee (gas+amount). Right now it is everybody's and nobody's responsibility and the concept is unclear. If gas simulation works good enough, fine. Then the extension/signer should do it.
3. signAndBroadcast should return the transaction that was actually signed including all the adjustments the signer did to the app. This is basically what I suggested here: Define common Wallet interface #264 (comment) (no matter if encoded as bytes or as an object)
4. Restore SigningCosmosClient.signAndBroadcast, which is a working public API – even if it just passes everything to the OnlineSigner. If needed we can ignore the fee argument if it turns out in 2. that the signer does that.

[ethanfrey]

Thank you for the feedback SImon.

I definitely agree on 1 and 4 and was thinking that already when I hit some issues.

For 3 adding the modified transaction (not just hash) is a nice one. Maybe Serialize Tx is the simplest interface to implement correctly

For 2. Yes, no need for the account number and sequence ever to be passed to an OnlineSigner. I can remove those as optional. For fees... I think the OnlineSigner should implement this - I will do. But maybe the app allows a user to manually override. I will make it the responsibility of the OnlineSigner, but accept a "hint" from the user/app id not needed

[Thunnini]

https://github.com/chainapsis/keplr-extension/blob/cosmjs/src/content-scripts/inject/cosmjs-provder.ts
https://github.com/chainapsis/keplr-example/blob/online-signer-demo/src/main.js

I made an initial prototype of online signer support for Keplr and basic example usage based on your PR.

For 2 of @webmaster128's opinion, I think that it is better to set sequence, account number field in the SigningCosmWasmClient. If sequence, account number is null, we have to fetch these values from the node in the online signer anyways, and this seems to be a duplicating the same logic.

And, it would be better if values ​​like apiUrl, broadcastMode are passed from SigningCosmWasmClient to the online signer
since these values ​​are duplicated in SigningCosmWasmClient and online singer, it is better to set them in one place.

[ethanfrey]

I think that it is better to set sequence, account number field in the SigningCosmWasmClient. If sequence, account number is null, we have to fetch these values from the node in the online signer anyways, and this seems to be a duplicating the same logic.

I think Simon's concern was that we do it maybe here, maybe there, rather than just leaving one place to do it, and I agree. In general, an app doesn't need to think about that stuff and I consider those values part of the signing process, so I moved that to 100% responsibility of the OnlineSigner. I think you do that already in Kepler, no? If this is an issue, we can move them 100% to the app and not allow nulls.

The only fields I think should be maybe signer, maybe app is fees. And in this case, I would propose the app can optionally set a gasWanted as it may have a good estimate of how much gas a complex tx will need. But leave the feesPerGas and final fees calculation to the signer.

[ethanfrey]

And, it would be better if values ​​like apiUrl, broadcastMode are passed from SigningCosmWasmClient to the online signer
since these values ​​are duplicated in SigningCosmWasmClient and online singer, it is better to set them in one place.

Yeah, I was thinking about the apiUrl in particular, and this actually ties with dynamically registering chains. I figured there would be some chainId -> apiUrl lookup in kepler, as it needs to have the chainId already registered to know the proper bech32 prefix and (maybe) hd derivation path. I am fine passing it through as well, but let's discuss this with a "dyamic chain registration" functionality. I think the better idea would have a registerChain(chainId, apiUrl, bech32prefix, hdPath, ...) method on the OnlineSigner that can add a new chain support, then we only need the chainId when calling signing. I know you have an issue about the dynamic registration, so maybe we can discuss more there and I think the required registration info is something more than I have above and less than what you have in your issue.

[ethanfrey]

Re: broadcast mode, does async (no confirmation) make any sense in a UI? I only see it for automated testing bots aiming for max volume.

That leaves us with sync and block as valid modes. I know that there have been issues with timeouts with the block choice, but it does make debugging a lot easier. I do see this as a good option to pass in.

[ethanfrey]

https://github.com/chainapsis/keplr-extension/blob/cosmjs/src/content-scripts/inject/cosmjs-provder.ts
https://github.com/chainapsis/keplr-example/blob/online-signer-demo/src/main.js

I made an initial prototype of online signer support for Keplr and basic example usage based on your PR.

Looking at these prototypes. Thank you.
My take-aways:

• The app definitely must set gas.
• I'm guessing that Keplr can set the fee amount if the gas is given
• Your extension never gets the accountNumber/sequence, but rather that is expected from the client. We can move the responsibility to the caller
• The extension doesn't post directly, but rather that is in the injected code.

I am a bit confused by the last two points, as it seems the extension doesn't go online to get info. But when looking at your ChainInfo proposal the extension needs both rpc and rest endpoints. The difference between the Online and Offline versions seems to be RequestTxBuilderConfigMsg vs. RequestSignMsg. And the tx builder just seems to be able to set fee price and take json -> sign bytes. But I don't see much difference there. Are there any docs (or code links) for the various request messages that the extension supports?

I guess my main question is what are the possible functionality modes that Keplr offers. And maybe the Online/Offline distinction doesn't make much sense. Rather we could have JSONTxSigner vs RawSignBytesSigner. Please let me know the functionality you would prefer to inject and I can make the CosmJS interfaces match.

[ethanfrey]

Just saw how you import branches from monorepos. Very cool!

[ethanfrey]

After looking more at the sample, I see all the critical work is done in the OfflineSigner:

• GetKeyMsg to get the address and public key
• RequestSignMsg to sign the SignBytes

The only addition seems to be the RequestTxBuilderConfigMsg which can set the fee before signing. It cannot set the gas needed, just the fee token. I assume this pops up some UI in the extension to control how much you pay.

This is actually quite nice, as we currently hard-code some price in the app and no way to adjust or predict it. However, if this is the only change we really need to make to the API, I have a much simpler solution than Online Signer. We just add another method to OfflineSigner to calculate fee for example.

interface OfflineFeeSigner extends OfflineSigner {
    readonly calculateFee: (chainId: string, gasLimit: number, suggestedFee?: Coin[]) => Promise<Coin[]>
}

It seems this is the heart of the current implementation. I could do something like:

export class CosmJSOfflineFeeSigner extends CosmJSOfflineSigner {
    async calculateFee(chainId: string, gasLimit: number, memo?: string, suggestedFee?: Coin[]): Promise<Coin[]> {

      const random = new Uint8Array(4);
      crypto.getRandomValues(random);
      const id = Buffer.from(random).toString("hex");

      const msg = new RequestTxBuilderConfigMsg(
        {
          chainId,
          accountNumber: 0,
          sequence: 0,
          memo,
          gas: gasLimit,
          fee: suggestedFee 
        },
        id,
        true,
        window.location.origin
      );

      const txBuilderConfig = await sendMessage(BACKGROUND_PORT, msg);
      
      let fee = txBuilderConfig.config.fee
        .split(",")
        .map(str => CosmosJsCoin.parse(str))
        .map(coin => {
          return {
            denom: coin.denom,
            amount: coin.amount.toString()
          };
        });

      return fee;
      // or we also take the memo
      // return { fee, memo: txBuilderConfig.config.memo);
    }
} 

This will just call the actual APIs you expose (and not require you to import a bunch of cosmjs code to get this to work). It simplifies the interfaces you need. And it makes it clear where the actual logic goes.

I am happy for any amount of logic and processing happening in the extension. And any amount in the app. I would like to keep the injected code as simple as possible. Especially as a version mismatch between the injected cosmjs and the app's cosmjs would not be so great and the less logic injected the better.

What do you think of this proposal?

[Thunnini]

Initially, I thought that the tx type can be not stdTx, so I designed that the setting the tx config and requesting the signature are separate. But, for now, I agree that the case of that the chain uses their custom transaction type doesn't exist and probably such case will not exist for a while. So, I also have plan to deal with the transaction by one step that just sets tx config and signing at once. Then, the difference between offlineSigner and onlineSigner would be small. IMHO, I think that the current onlineSigner is enough and it can replace the offlineSigner.

In current Keplr's onlineSigner implementation, I prohibited the null gas field because the gas simulation is not implemented yet, so we can't calculate the gas automatically. Users can change the gas on extension's UI if the tx config is requested.

The extension doesn't post directly, but rather that is in the injected code. The answer to this line can be slightly complex. As you said, in the current implementation, the transaction is not broadcasted by the extension. But, if you have used the Keplr, you can see the transaction's status is notified by the browser's notification system and it works even in the wallet.keplr.app webpage. This feature is not described yet, Keplr extension can be delegated to broadcast the transaction on behalf of the webpage with using the browser's notification system. So, I think I can provide the option that the transaction is broadcasted by the webpage or delegate the broadcasting to the extension. If the transaction broadcasting is delegated to the extension, this transaction will be broadcasted to the node that Keplr knows.

The only fields I think should be maybe signer, maybe app is fees. And in this case, I would propose the app can optionally set a gasWanted as it may have a good estimate of how much gas a complex tx will need. But leave the feesPerGas and final fees calculation to the signer.

@ethanfrey I'm not sure that I understood this part 100%. Could you clarify this part?

[ethanfrey]

Thank you for clarifying. From looking at the UI, it is hard to see what is done where exactly.

So, I think I can provide the option that the transaction is broadcasted by the webpage or delegate the broadcasting to the extension. If the transaction broadcasting is delegated to the extension, this transaction will be broadcasted to the node that Keplr knows.

Okay, if you will be adding the broadcast by the extension in the future (to the node Keplr knows), I would stick with this interface. And when implemented make cosmjs just use that part. I feel rather hesitant to have so much code compiled into the injected code (as imports). Maybe we can trim that down later, but I was wondering if it made sense the functionality.

The only fields I think should be maybe signer, maybe app is fees. And in this case, I would propose the app can optionally set a gasWanted as it may have a good estimate of how much gas a complex tx will need. But leave the feesPerGas and final fees calculation to the signer.

@ethanfrey I'm not sure that I understood this part 100%. Could you clarify this part?

Sorry, re-reading that is confusing. I wanted to make sure either the app or the signer is responsible for all fields. Does this make sense:

The app provides:

• messages
• chain_id
• gasLimit
• memo?
• fees?

The signer provides:

• account_number
• sequence
• fees

The signer may override:

• memo
• gasLimit

[ethanfrey]

Given your feedback, I would just polish off these types a bit

[ethanfrey]

@webmaster128 is on vacation and I would like his input for the final types. I see two main options.

Option 1

The first one is to just make the relatively minor adjustments to the current OnlineSigner interface as described above: #403 (comment)

This would give us:

// move more fields to the caller
export interface SignRequest {
  // required fields
  readonly msgs: readonly Msg[];
  readonly chainId: string;
  readonly gas: string;
  readonly account_number: string | number;
  readonly sequence: string | number;
  // optionally set by caller, can be overriden by signer
  readonly fee_amount?: readonly Coin[];
  readonly memo?: string;
}

export interface SignAndBroadcastResult {
  // we add this field as well (return the signed tx that was broadcast) - definition in option 2
  tx: StdTx;
  result: BroadcastTxResult;
}

export interface OnlineSigner {
  readonly enable: () => Promise<boolean>;
  readonly getAccounts: () => Promise<readonly AccountData[]>;
  // pass in broadcast mode when calling
  readonly signAndBroadcast: (address: string, request: SignRequest, broadcastMode = BroadcastMode.Block) => Promise<SignAndBroadcastResult>;
}

[ethanfrey]

Option 2

When looking at the implementation in keplr, I notice they have to pull in a lot of @cosmjs/launchpad code into the injected script to make this work. As the extension does not directly submit the transaction. I would make a new interface tailored to just what the extension actually does. If it starts broadcasting in a later version, we can revive the OnlineSigner interface.

Here is an alternate design (freezing this PR and replacing it with a new design). It should be easy to update the current Keplr injected script to this new API (just trim down a bit), as I describe above: #403 (comment)

We would have a slightly higher-level interface (chain-aware) that can make some tx modifications before signing.

// minimal
export interface OfflineSigner {
  readonly getAccounts: () => Promise<readonly AccountData[]>;
  readonly sign: (address: string, message: Uint8Array, prehashType?: PrehashType) => Promise<StdSignature>;
}

// this can modify data and returns a complete StdTx with a valid Signature
export interface BuildingSigner {
  readonly getAccounts: () => Promise<readonly AccountData[]>;
  readonly enable: () => Promise<boolean>;
  readonly buildAndSign: (address: string, request: SignRequest) => Promise<StdTx>;
}

export interface StdTx {
  readonly msg: readonly Msg[];
  readonly fee: StdFee;
  readonly signatures: readonly StdSignature[];
  readonly memo: string | undefined;
}

export interface StdSignature {
  readonly pub_key: PubKey;
  readonly signature: string;
}

export interface PubKey {
  readonly type: string;
  // Value field is base64-encoded in all cases
  readonly value: string;
}

We could use the exact same SignRequest as in Option 1, except a different return value. Most of the imported code and this broadcastMode argument are doing stuff in the injected script that is not done in the extension and could better be done in an app. The cosmJSprovider (and extension) will return the exact signed tx that will be broadcast, so it can also precompute the txHash and monitor the transaction (it seems this is the current flow in cosmosJSprovider).

[ethanfrey]

Given that the only clear use-case we have for the OnlineSigner or BuildingSigner interface is Keplr, I think it makes sense to tailor it to Keplr's interface and exposed capabilities, which would be quite close to BuildingSigner. It's more powerful and generic than the "calculateFeeSigner", as it lets the extension make any desired transformations and sign the final transaction.

The app retains the responsibility of broadcasting and error handling there, which is much simpler than trying to feed the entire error handling from broadcasting a tx from the extension into the app. Option 2 is my preference, and I would be happy to code it when I am back (in one week), but I would like to hear the opinions of @Thunnini and @webmaster128 first. I think this would be an easy adjustment to what you have already built on the cosmjs integration branch.

[Thunnini]

Wouldn't it be better if the sequence was optional so it can also be set by the signer?

// move more fields to the caller
export interface SignRequest {
  // required fields
  readonly msgs: readonly Msg[];
  readonly chainId: string;
  readonly gas: string;
  readonly account_number: string | number;
  readonly sequence: string | number;
  // optionally set by caller, can be overriden by signer
  readonly fee_amount?: readonly Coin[];
  readonly memo?: string;
}

I do think option 2 is good but personally prefer option 1.
While we’re not committed to a specific design, I do think option 1 allows more flexibility. For example, the wallet can provide the features that account for cases where the transaction failed due to lack of gas and the wallet may be able to try again with higher gas. Another case I could think of is the frontend wanting to send multiple transactions at the same time, and the transactions can be stored as a queue to be set the sequence in order that the transaction succeeds. I think option 1 allows more options for more creative wallet designs so I do like option 1 better.

[webmaster128]

I scanned most of the discussion now and a core question seems to be still open: which component is responsible for broadcasting a transaction? As long is there is no consensus on this one, all the rest cannot efficiently be worked on.

The two basic ways are:

• A: (OnlineSigner) The signer signs the transaction and broadcasts it, informing the frontend what it did (this is shown in this PR)
• B: (OfflineSigner) The signer signs the transaction, potentially adjusting some values and the frontend broadcasts it (this is what a Ledger signer does and what Keplr does so far)

If we want A, then Keplr should do the broadcasting. If we want B, then we can close here and improve the existing OfflineSigner interface.

When looking at the implementation in keplr, I notice they have to pull in a lot of @cosmjs/launchpad code into the injected script to make this work. As the extension does not directly submit the transaction. I would make a new interface tailored to just what the extension actually does. If it starts broadcasting in a later version, we can revive the OnlineSigner interface.

+1 for this. The OfflineSigner is simpler, exists already and works for Ledger. As long as we don't have a good, specific reason to move broadcasting from the frontend to the extension, I would not do it.

While we’re not committed to a specific design, I do think option 1 allows more flexibility. For example, the wallet can provide the features that account for cases where the transaction failed due to lack of gas and the wallet may be able to try again with higher gas. Another case I could think of is the frontend wanting to send multiple transactions at the same time, and the transactions can be stored as a queue to be set the sequence in order that the transaction succeeds. I think option 1 allows more options for more creative wallet designs so I do like option 1 better.

Option A (~ Option 1) creates more flexibility for the signer while Option B (~ Option 2) creates more flexibility in the frontend. So far I did not hear a convincing argument for A/1 over B/2 other than the fact that a frontend can easily trick the user into signing multiple transactions by presenting fake error messages and then broadcasting all of them to perform a payment multiple times.

[webmaster128]

Thank you Ethan for proposing this and @Thunnini for all the input. The way of wrapping an OfflineSigner in an OnlineSigner implementation is very neat and will not be forgotten.

For now we decided to allow signers to override parts of the sign doc without implementing an OnlineSigner (#432). Thus #316 remains open until we have a very good reason to implement it. Closing here.