mymonero / mymonero-core-js

The JS library containing the Monero crypto plus lightwallet functions behind the official MyMonero apps
BSD 3-Clause "New" or "Revised" License
101 stars 103 forks source link

Create tx with pre-generated txkey #48

Closed marceloneil closed 6 years ago

marceloneil commented 6 years ago

Since the tx private key is irrecoverable, I think it makes sense to provide some method of storing it. This PR adds an optional parameter to the create_transaction() and construct_tx() functions called txkey, so that you can generate and save the txkey, and use it to generate a transaction.

I'm unsure about how the branches work on this repo, so let me know if it's better to do a PR against develop

paulshapiro commented 6 years ago

Hey there, yes, this is cool with me, and actually something I'm doing with the new C++ stuff. So just a heads up that we'll need to do this again. However I do want to ask you to explain the necessity to pass in the tx_key. It's not a bad thing, but it does open up the possibility that some unknowing integrator will pass a bad value. It would make sense if you want to control the source of random numbers but I don't see that being necessary (and therefore a good idea) here.

paulshapiro commented 6 years ago

And yes, definitely do branch from develop please. It's basically the Gitflow model

marceloneil commented 6 years ago

My logic was you could do something along these lines:

const txkey = monero_utils.random_keypair()
const tx = monero_utils.create_transaction(...params, txkey)
// If you are using a subaddress, maybe modify txkey.pub or just get it from tx.extra
storeTxAndKey(tx, txkey)

I wanted to keep the code backwards compatible since it's such a small change, so returning the tx and txkey seemed like it might be a bit much. I can write some sort of check for txkey if that sounds good, but I'm more than happy to change my approach.

paulshapiro commented 6 years ago

Hi Marcel, you could definitely tack on another parameter (tx key) in the success callback to SendFunds, and return the same however you want from cryptonote_utils.

marceloneil commented 6 years ago

Well doesn't SendFunds implement cryptonote_utils anyways? So you would still need a way to pass through/retrieve the tx key from cryptonote_utils

paulshapiro commented 6 years ago

Right. In any case, API changes happen. We ourselves inherited this original codebase. So I think the best we can do is prepare it to scale. But for what it's worth, have you seen what I'm doing with the serial_bridge_index? I'm sorry to say I had to take some time off over the weekend so I didn't get done my create_transactions C++ -> JS bridging of the final method, create_transactions, but I will have that ready to show you soon. It's trivial to return the tx key. Here's an example of returning multiple values.

marceloneil commented 6 years ago

Take a peak at #49 and see if you like my new changes