Skip to content

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
7 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.

5 participants