Skip to content

Fixed transaction signatures order.#84

Open
zorba128 wants to merge 3 commits intoskynetcap:mainfrom
zorba128:signatures
Open

Fixed transaction signatures order.#84
zorba128 wants to merge 3 commits intoskynetcap:mainfrom
zorba128:signatures

Conversation

@zorba128
Copy link
Contributor

Improved Message to support external signers.
Also some refactorings be able to use it outside of Transaction/TransactionBuilder.

Signed-off-by: Marcin Kielar <zorba128@interia.pl>
Signed-off-by: Marcin Kielar <zorba128@interia.pl>
….serialize` is called without preceding it with `Transaction.sign`.

Signed-off-by: Marcin Kielar <zorba128@interia.pl>
@zorba128
Copy link
Contributor Author

zorba128 commented Jan 4, 2025

Also updated Transaction/TransactionBuilder signature handling to ensure correct signature order. This should make it possible to correctly sign non-trivial transactions that involve multiple instructions/accounts.

@zorba128 zorba128 changed the title Refactored Message to support external signers Fixed transaction signatures order. Jan 4, 2025
@zorba128
Copy link
Contributor Author

Any chance? Or maybe discussion? This is really broken, with transaction that requires few signatures it is impossible now to guess what to pass to sign method... Just try to make transaction with memo and few different token transfers.

@freedombird9
Copy link

@zorba128 It still requires signers when transaction.serialize(). If no signer is set, it throws RuntimeException("Missing signer for account " + publicKey). I don't understand how this commit sovles the issue.

@zorba128
Copy link
Contributor Author

  1. At least you know which signature is missing, rather than cryptic null pointer exception
  2. The thing here is to allow specifying signatures without messing up account order in the transaction. In original design signing affected accounts, making it hard/impossible to reason about outcome, and often yielding invalid transactions.
    This comes into play when using tokens (with main account to be used for fee is different than source account). Or memo.

@freedombird9
Copy link

freedombird9 commented Apr 22, 2025

@zorba128 Yes, much appreciated. I further modified it to serialize without any signature, making external sign possible:

           // if signers is empty 
           int signaturesSize = 0; // No signatures
           byte[] signaturesLength = ShortvecEncoding.encodeLength(signaturesSize);
           int totalSize = signaturesLength.length + serializedMessage.length;
           ByteBuffer out = ByteBuffer.allocate(totalSize);
           out.put(signaturesLength);
           out.put(serializedMessage);

           return out.array();

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants