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

Be more careful about endianness with Identity and Address #1926

Merged
merged 11 commits into from
Nov 5, 2024

Conversation

kazimuth
Copy link
Contributor

Description of Changes

This fixes some issues caused by #1616.

We are now more careful about endianness when converting Identity and Address to byte arrays / strings.

API and ABI breaking changes

ABI break; this will require a wipe of modules, since the controldb has been changed to look up identities and addresses by their usual printed form, which corresponds to a big-endian byte array.

Expected complexity level and risk

5/5

Testing

See the added tests, also running smoketests.
I am not sure what else would be a good idea to do.

@kazimuth
Copy link
Contributor Author

This does not presently fix the incompatibility between the serde and sats serialization of Identity and Address. I will leave that for a later change if needed.

Copy link
Contributor

@cloutiertyler cloutiertyler left a comment

Choose a reason for hiding this comment

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

This is great. Just what the doctor ordered I think. I left a few small comments. Keen to hear about the motivation for changing the control db key serialization.

@cloutiertyler
Copy link
Contributor

This does not presently fix the incompatibility between the serde and sats serialization of Identity and Address. I will leave that for a later change if needed.

Can you be more specific about what incompatibility you mean here?

@kazimuth
Copy link
Contributor Author

kazimuth commented Nov 1, 2024

Can you be more specific about what incompatibility you mean here?

Yes, sats+serde_json serializes Identity as {"__identity__": "0x[hex]"} whereas serde+serde_json serializes Identity as merely "[hex]".

This is slightly silly because we already have the IdentityForUrl type in client-api that is defined to serialize as "[hex]".

@kazimuth
Copy link
Contributor Author

kazimuth commented Nov 1, 2024

On reflection I don't think this needs anything else, besides possibly additional testing, but I'm not sure what that testing would entail.

@bfops bfops added abi-break A PR that makes an ABI breaking change release-rc1 labels Nov 4, 2024
@kazimuth
Copy link
Contributor Author

kazimuth commented Nov 4, 2024

Okay, I think I've got everything and this is ready.

Copy link
Contributor

@cloutiertyler cloutiertyler left a comment

Choose a reason for hiding this comment

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

This LGTM but internal tests are failing.

@kazimuth kazimuth enabled auto-merge November 5, 2024 20:03
@kazimuth kazimuth added this pull request to the merge queue Nov 5, 2024
Merged via the queue into master with commit a24a206 Nov 5, 2024
8 checks passed
@kazimuth kazimuth deleted the jgilles/endianness branch November 5, 2024 20:48
@bfops bfops restored the jgilles/endianness branch November 5, 2024 21:38
@bfops bfops deleted the jgilles/endianness branch November 5, 2024 21:38
@kazimuth kazimuth mentioned this pull request Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
abi-break A PR that makes an ABI breaking change release-rc1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants