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

Switch pallet-bioauth authorities to WeakBoundedVec and enable generate_storage_info #208

Merged
merged 9 commits into from
Dec 17, 2021

Conversation

dmitrylavrenov
Copy link
Contributor

Closes #187

@dmitrylavrenov dmitrylavrenov marked this pull request as draft November 23, 2021 13:16
Copy link
Contributor

@MOZGIII MOZGIII left a comment

Choose a reason for hiding this comment

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

Why can't we just make the AuthTicketNonce type the WeakBoundedVec instead of Vec, and leave it in the pallet?

@dmitrylavrenov
Copy link
Contributor Author

Why can't we just make the AuthTicketNonce type the WeakBoundedVec instead of Vec, and leave it in the pallet?

In case we use AuthTicketNonce as WeakBoundedVec it provides getting an error due to genesis_config requires all fields be serialized and deserialized.

pub consumed_auth_ticket_nonces: Vec<AuthTicketNonce<T>>, the trait Serialize is not implemented for WeakBoundedVec<u8, <T as pallet::Config>::MaxAuthTicketNonce

The task is to define a type for AuthTickenNonce that will have a limited size and be serialized and deserialized. That's why for now I exposed it to the runtime level where it can be implemented.

According the fact that [u8; 32] is serialized and deserialized we can use it at pallet level easly.

@MOZGIII
Copy link
Contributor

MOZGIII commented Nov 24, 2021

I think this is ultimately a good time to take into account the refactoring of our nonces system.
Currently, they are just integers under the hood, but what we really want, I think, is a salted hash of the said value to make it look more unique. I'd like to hear your thoughts on this though. If we go with this - then we can switch to constant-size arrays.
Alternatively, if we decide to go without obscuring the nonce values, we can start representing them via the actual integers - i.e. u128 vs [u8; N].

@dmitrylavrenov
Copy link
Contributor Author

dmitrylavrenov commented Nov 24, 2021

I think this is ultimately a good time to take into account the refactoring of our nonces system.
Currently, they are just integers under the hood, but what we really want, I think, is a salted hash of the said value to make it look more unique. I'd like to hear your thoughts on this though. If we go with this - then we can switch to constant-size arrays.
Alternatively, if we decide to go without obscuring the nonce values, we can start representing them via the actual integers - i.e. u128 vs [u8; N].

In terms of nonces, I like to use it as a cryptographic object to make it more unique. A salted hash os one of them as well. For example for now we can use SHA256 to generate nonces from salt. 256 bits hash is 32 bytes. Therefore, [u8; 32] can be used for now. What do you think?

@MOZGIII
Copy link
Contributor

MOZGIII commented Nov 24, 2021

I'd like to discuss the pros and cons of going with obscured vs plain nonce; regarding the hash function itself - we probably should discuss it later, but I think we need to go with something like keccak, rather than sha2...

@dmitrylavrenov dmitrylavrenov marked this pull request as ready for review December 2, 2021 09:35
Copy link
Contributor

@MOZGIII MOZGIII left a comment

Choose a reason for hiding this comment

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

See comments

@dmitrylavrenov dmitrylavrenov marked this pull request as draft December 13, 2021 09:15
@MOZGIII
Copy link
Contributor

MOZGIII commented Dec 17, 2021

I think this is ready to merge, let's do it?

@dmitrylavrenov dmitrylavrenov marked this pull request as ready for review December 17, 2021 09:20
@dmitrylavrenov
Copy link
Contributor Author

I think this is ready to merge, let's do it?

Just doing it.

@dmitrylavrenov dmitrylavrenov merged commit a110236 into master Dec 17, 2021
@dmitrylavrenov dmitrylavrenov deleted the bioauth-storage-info branch December 17, 2021 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Switch pallet-bioauth authorities to WeakBoundedVec and enable generate_storage_info
2 participants