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

Implement efficient conversion from String, Vec<u8> and &[u8] for identifiers #959

Open
mina86 opened this issue Nov 10, 2023 · 0 comments
Labels
O: optimization Objective: aims to optimize performance, allocations and computations

Comments

@mina86
Copy link
Contributor

mina86 commented Nov 10, 2023

Let’s look at ClientId for example. Suppose I have a client id as a String. There’s currently no way to convert that String into ClientId without performing an allocation. The two available constructors are ClientId::from_str (which takes a str slice and then allocates a string) and Client::new (which takes client type and counter and than formats a new id).

The feature request is to provide TryFrom implementations for the identifiers which take ownership of the passed String.

Going further, I’d also love to see constructors which take Vec and &[u8] arguments and skip UTF-8 verification checking. Currently, user has to first get a String or &str and then pass that to identifier’s constructor. However, constructors already check if the string consists of valid characters so they might work on byte slices just as easily and after performing verification convert them to strings.

mina86 added a commit to mina86/ibc-rs that referenced this issue Nov 10, 2023
Firstly, remove asserts from the validate_identifier_length and
validate_prefix_length functions.  Instead, let the regular checks
handle cases where min and max constraints don’t allow for a valid
prefix to exist.

Secondly, ensure minimum length constraints is at least one.  This
makes sure that the validation functions will check for empty
identifiers and prefixes.  This makes empty identifier check in
validate_channel_identifier as well as Error::Empty variant
unnecessary so get rid of those too.

Lastly, collapse all checks in validate_prefix_length into a single
call to validate_identifier_length with correctly adjusted min and max
constraints.

Issue: cosmos#959
mina86 added a commit to mina86/ibc-rs that referenced this issue Nov 10, 2023
Firstly, remove asserts from the validate_identifier_length and
validate_prefix_length functions.  Instead, let the regular checks
handle cases where min and max constraints don’t allow for a valid
prefix to exist.

Secondly, ensure minimum length constraints is at least one.  This
makes sure that the validation functions will check for empty
identifiers and prefixes.  This makes empty identifier check in
validate_channel_identifier as well as Error::Empty variant
unnecessary so get rid of those too.

Lastly, collapse all checks in validate_prefix_length into a single
call to validate_identifier_length with correctly adjusted min and max
constraints.

Issue: cosmos#959
rnbguy added a commit that referenced this issue Nov 13, 2023
* refactor: simplify identifier length validation

Firstly, remove asserts from the validate_identifier_length and
validate_prefix_length functions.  Instead, let the regular checks
handle cases where min and max constraints don’t allow for a valid
prefix to exist.

Secondly, ensure minimum length constraints is at least one.  This
makes sure that the validation functions will check for empty
identifiers and prefixes.  This makes empty identifier check in
validate_channel_identifier as well as Error::Empty variant
unnecessary so get rid of those too.

Lastly, collapse all checks in validate_prefix_length into a single
call to validate_identifier_length with correctly adjusted min and max
constraints.

Issue: #959

* log

* Update 960-simplify-length-validation.md

Signed-off-by: Michal Nazarewicz <mina86@mina86.com>

* tests

* Update and rename 960-simplify-length-validation.md to 961-simplify-length-validation.md

Signed-off-by: Michal Nazarewicz <mina86@mina86.com>

* update changelog entry

* revert error variant deletion

* refactor id validation tests

* use numeral consistently

---------

Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
Co-authored-by: Ranadeep Biswas <mail@rnbguy.at>
@Farhad-Shabani Farhad-Shabani added the O: optimization Objective: aims to optimize performance, allocations and computations label Nov 20, 2023
Farhad-Shabani pushed a commit that referenced this issue Sep 9, 2024
* refactor: simplify identifier length validation

Firstly, remove asserts from the validate_identifier_length and
validate_prefix_length functions.  Instead, let the regular checks
handle cases where min and max constraints don’t allow for a valid
prefix to exist.

Secondly, ensure minimum length constraints is at least one.  This
makes sure that the validation functions will check for empty
identifiers and prefixes.  This makes empty identifier check in
validate_channel_identifier as well as Error::Empty variant
unnecessary so get rid of those too.

Lastly, collapse all checks in validate_prefix_length into a single
call to validate_identifier_length with correctly adjusted min and max
constraints.

Issue: #959

* log

* Update 960-simplify-length-validation.md

Signed-off-by: Michal Nazarewicz <mina86@mina86.com>

* tests

* Update and rename 960-simplify-length-validation.md to 961-simplify-length-validation.md

Signed-off-by: Michal Nazarewicz <mina86@mina86.com>

* update changelog entry

* revert error variant deletion

* refactor id validation tests

* use numeral consistently

---------

Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
Co-authored-by: Ranadeep Biswas <mail@rnbguy.at>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O: optimization Objective: aims to optimize performance, allocations and computations
Projects
Status: 📥 To Do
Development

No branches or pull requests

2 participants