Improve transaction signing/submission flow

[mDuo13]

There are a lot of different ways of doing transaction signing and submission, but we've learned from experience what models work better than others, and the way I most recommend doing things is as follows:

1. Prepare Transaction (online)

   The main point of this is that you can inspect what you're signing before you even go get your secret keys out of secure storage. For offline transaction construction, you have to skip this step and provide things like Fee and Sequence yourself. (The Wallet class in xrpl-py is well-equipped to handle the Sequence part at least.)

   Input:

   • Incomplete transaction instructions (JSON or otherwise)
   • network client
   • (optional) instructions for auto-filling, such as max ledger offset (e.g. a number to add to the latest validated ledger to get LastLedgerSequence), fee options, path options, etc.

   Output:

   • Complete transaction instructions (JSON or otherwise) with all required signing fields filled out, X-addresses converted to classic address & tag, and any other preprocessing.
     • Should provide a sensible LastLedgerSequence field by default, as well as any other best practices, with options to explicitly configure/disable them.

2. Sign Transaction (offline OK)

   Input:

   • Complete transaction instructions (JSON or otherwise) with all required signing fields filled out
   • Seed or private key

   Output:

   • Signed transaction blob
   • (Optional) Signed transaction JSON.
   • Transaction ID (hash).

3. Submit-and-Verify (online)

   It's not a bad thing to also have a simple submit function that returns the preliminary result only.

   Input:

   • Signed transaction blob (or, potentially, signed transaction in JSON format) with a LastLedgerSequence parameter.
   • (Optional) Minimum ledger index. If not provided, assume that the minimum ledger is the latest validated ledger +1 at time of submission. (Note: Not the current open ledger; during times of instability, there could be several closed but unvalidated ledgers, and a transaction could theoretically end up getting into a replacement for one of the closed ledgers.) This is useful because you can use this function to resubmit the signed transaction any number of times and it'll still work consistently and idempotently as long as you always provide the same minimum ledger index. To implement truly robust transaction submission, one would need to look up and save the original min ledger index alongside the signed blob before submitting the transaction at all.

   Output:

   • The transaction's final result code. Optionally it could raise an exception if the result code is anything other than tesSUCCESS.

---

There are several ways in which the current version of xrpl-py DOES NOT work this way:

• What little auto-filling functionality exists is tied into signing. It should be separated out into a prepare function. Though, it's probably also fine if the X-address conversion happens during signing since that's deterministic and does not require a network connection.
• The auto-filling functionality should be more robust, such as providing a LastLedgerSequence by default and filling Fee and Sequence.
  • I was surprised, safe_sign_transaction() increases your wallet's sequence number tracking but doesn't actually add the field to the transaction! Increasing the sequence number and attaching it to a transaction should probably happen around the same time, preferably in the Prepare Transaction step instead of signing.
• The signing functionality should ensure that the transaction has all required fields so you don't end up signing and trying to submit an incomplete transaction. Submitting a signed but incomplete transaction is embarrassing and could be tricky for users to catch since the error only arises much later than the thing that caused it. Arguably it could also weaken security since it means you've created more signatures which gives people more data to use to try and break your keys.
• send_reliable_submission() takes a wallet and unsigned transaction. That gives the impression that it might modify the transaction instructions and (re-)submit a replacement/modified version, which it absolutely MUST NOT do. (I don't think it does, but it's best if the function literally doesn't have what it would need to do that.) You can't use the existing function to implement truly reliable sending.

[ledhed2222]

what do you think about this solution:

1. safe_sign_transaction will check whether values in the Tx need to be autofilled. If they do, it will call some private functions to fill seq num and fee.
2. send_reliable_submission will raise if the Tx is not signed

This way we add no new methods to the public interface, and I believe we will resolve your concerns.

[ledhed2222]

i see what you'll find problematic about that solution ^

@mDuo13 - would you elaborate on:

send_reliable_submission() takes a wallet and unsigned transaction. That gives the impression that it might modify the transaction instructions and (re-)submit a replacement/modified version, which it absolutely MUST NOT do. (I don't think it does, but it's best if the function literally doesn't have what it would need to do that.) You can't use the existing function to implement truly reliable sending.

namely - why does it give you the impression that a replacement tx will happen?

[mDuo13]

send_reliable_submission() takes a wallet and unsigned transaction. That gives the impression that it might modify the transaction instructions and (re-)submit a replacement/modified version, which it absolutely MUST NOT do. (I don't think it does, but it's best if the function literally doesn't have what it would need to do that.) You can't use the existing function to implement truly reliable sending.

namely - why does it give you the impression that a replacement tx will happen?

For one thing, that's how Ripple-REST worked. But for another thing, the mere fact that it could do this is cause for concern. The code patterns should encourage and reinforce safe usage, and what's safe is to sign once using purely offline code, and then submit the exact same signed blob any number of times.

[mDuo13]

On a related note, we should make sure that the methods we have can be integrated with hardware wallets (like a Ledger Nano S). I don't have one so I don't know exactly how they work, but I know they have a companion app and the secret lives on the device. I don't know if they handle things like actually submitting to the network or if they just output a binary blob you have to submit yourself, but that's something to check into and make sure we can work with (even if you have to do some parts manually).

[mvadari]

Are these concerns still existent?

[intelliot]

@pdp2121 is working on this here: #528

[JST5000]

I believe this is now resolved thanks to #528