manuelmauro/algonaut

`Account::sign_transaction` should *not* take a reference to `Transaction`.

Closed this issue · 5 comments

Is your feature request related to a problem? Please describe.
sign_transaction clones the Transaction being passed in. This cost should not be hidden from the user.

Describe the solution you'd like
Change the reference to the underlying type.

Describe alternatives you've considered
None. It's pretty straightforward.

Additional context
Sorry for the sudden flood of issues. Been working on a bunch of things lately, so I'm working through a list of things I'd jotted down. This the last one though, so it's a short list :D

:D Thank you for your work! Sounds a considerate change to me. Please feel free to send a pull request.

I agree - it also makes sense to transfer ownership to the function / signed tx, as you normally don't want to continue using the original tx. If you need it, it's in the signed tx.

I'll go ahead and make this change, then.

There's another function, one estimate_txn_size or something similar, that estimates by simply signing the transaction and packing the bytes.

That should probably get a "better" implementation once things kind of stabilize about how transactions work.

There's another function, one estimate_txn_size or something similar, that estimates by simply signing the transaction and packing the bytes.

That should probably get a "better" implementation once things kind of stabilize about how transactions work.

I commented partly about this in #131
The Go SDK serializes the transaction and adds a fixed number of bytes. It's a bit unclear how this function is meant to be used, especially because multi sig and logic sig transactions have varying sizes, so I left it unchanged for now.

Note that the current implementation also crashes when called in WASM (because it uses ring). I fixed it locally with an implementation similar to the Go one. Could upload this.