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

did-simple: implement pubkey access with fixed size arrays #98

Merged
merged 2 commits into from
May 20, 2024

Conversation

TheButlah
Copy link
Contributor

@TheButlah TheButlah commented May 19, 2024

Rebased on #100, merge that first.

I'm not sure about this PR. I may have overcomplicated things.

The goal was to make it so that the pubkey is always returned as a reference to a fixed-size array.
This is because key algos specify up front their length, so we want to show that we validated that length.
However, I'm not sure that the type machinery I made is user friendly. What are your thoughts @kayhhh @MalekiRe @Schmarni-Dev?

@TheButlah TheButlah marked this pull request as draft May 19, 2024 06:14
@TheButlah TheButlah changed the base branch from main to thebutlah/improve_decode_implement_pubkey May 19, 2024 06:18
@TheButlah TheButlah force-pushed the thebutlah/improve_decode_implement_pubkey branch from 0c19d66 to f10c617 Compare May 19, 2024 06:56
Base automatically changed from thebutlah/improve_decode_implement_pubkey to thebutlah/encode_varint May 19, 2024 06:58
@TheButlah TheButlah changed the base branch from thebutlah/encode_varint to thebutlah/decode_varint May 19, 2024 07:01
@TheButlah TheButlah force-pushed the thebutlah/decode_varint branch 2 times, most recently from f78960f to c92fb75 Compare May 19, 2024 07:13
@TheButlah TheButlah force-pushed the thebutlah/implement-pubkey-access branch from 3fd716e to 32019d0 Compare May 19, 2024 07:14
Copy link
Contributor

@Schmarni-Dev Schmarni-Dev left a comment

Choose a reason for hiding this comment

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

LGTM

Base automatically changed from thebutlah/decode_varint to main May 20, 2024 03:40
@TheButlah TheButlah force-pushed the thebutlah/implement-pubkey-access branch 3 times, most recently from 7f6a1db to bb315cb Compare May 20, 2024 20:37
@TheButlah TheButlah marked this pull request as ready for review May 20, 2024 20:38
@TheButlah
Copy link
Contributor Author

After getting feedback from @kayhhh that the api looks unecessarily complicated, I've dumbed it down and gotten rid of the fixed size arrays.

@TheButlah TheButlah force-pushed the thebutlah/implement-pubkey-access branch from 354dd6e to bb2d14e Compare May 20, 2024 20:58
@TheButlah TheButlah force-pushed the thebutlah/implement-pubkey-access branch 2 times, most recently from fb0054a to f58e99d Compare May 20, 2024 21:10
@TheButlah TheButlah force-pushed the thebutlah/implement-pubkey-access branch from f58e99d to 1007fa0 Compare May 20, 2024 21:17
Copy link

@kayhhh kayhhh left a comment

Choose a reason for hiding this comment

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

👍

@TheButlah TheButlah merged commit 828cadc into main May 20, 2024
3 checks passed
@TheButlah TheButlah deleted the thebutlah/implement-pubkey-access branch May 20, 2024 21:26
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.

3 participants