Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

payload signer signatures #417

Merged

Conversation

sreuland
Copy link
Contributor

@sreuland sreuland commented Apr 8, 2022

As part of CAP-40, needed to include payload signer signature generation from KeyPair and separate way to add pre-built signatures onto a Transaction.

…enabled adding pre-built signatures onto a transaction
Copy link
Contributor

@leighmcculloch leighmcculloch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two questions (❓).

Comment on lines 273 to 276
//XOR the new hint with this keypair's public key hint
for (int i = 0; i < hint.length; i++) {
hint[i] ^= (i < payloadSignature.getHint().getSignatureHint().length ? payloadSignature.getHint().getSignatureHint()[i] : 0);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ I'm a bit confused by this. It looks like there's a condition here to handle the payloadSignature having a too short hint, however the too short hint is handled a few lines above already. Hints are always 4 bytes. A SignatureHint should never contain a hint shorter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

payloadSignature.getHint().getSignatureHint() refers to the key hint, it was paranoid defensive guard on that key hint array being shorter than the payload hint array, fine with removing it for cleaner approach here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think you can remove the check. Hint's should always be 4 bytes. In other languages where we can guarantee the length of the byte[] is 4 we do that, I guess in Java we can't?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

java array type doesn't include the dimension as part of the type declaration and the generated xdr.SignatureHint doesn't have any checks on the size. it would require decorator or other wrapper class in the sdk around xdr.SignatureHint that enforces the length dimension. I removed that guard check.

Comment on lines 118 to 121
(byte)(255),
(byte)(0xFF & 64),
(byte)(0xFF & 7),
(byte)(0xFF & 55)});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ What is the 0xFF here for? Isn't 0xff & 64 = 0x64? I think you can simply do:

Suggested change
(byte)(255),
(byte)(0xFF & 64),
(byte)(0xFF & 7),
(byte)(0xFF & 55)});
(byte)(255),
(byte)(64),
(byte)(7),
(byte)(55)});

See this as an example: https://go.dev/play/p/qi-HubnfmPA

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, wasn't necessary, wanted to show the 8 bit unsigned value being placed into each byte, and did used that form for all values even the ones below 128

@sreuland sreuland requested a review from leighmcculloch April 8, 2022 21:42
@sreuland sreuland merged commit f61a675 into lightsail-network:protocol_19 Apr 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants