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

feat!: hex string length support #213

Merged
merged 7 commits into from
Mar 19, 2021
Merged

feat!: hex string length support #213

merged 7 commits into from
Mar 19, 2021

Conversation

AuHau
Copy link
Contributor

@AuHau AuHau commented Mar 17, 2021

Supersedes #201 PR.

Breaking changes:

  • HexString type is considered as non-prefixed hex string and all functions using it are expecting to have it like that. If used on public API, then it should be sanitized with makeHexString function
  • dropped stripHexPrefix in favor of makeHexString
  • Hex utility functions no longer take withPrefix: boolean flag as they always output non-prefixed hex and instead take len?: number to assert expected length of the non-prefixed hex string
  • Topic type was changed from Bytes into HexString

@AuHau AuHau force-pushed the feat/hex-string branch from 8a06987 to e966ad6 Compare March 17, 2021 10:13
@AuHau AuHau changed the title refactor: expand generic variable names feat!: hex string length support Mar 17, 2021
src/utils/eth.ts Outdated Show resolved Hide resolved
src/utils/eth.ts Outdated Show resolved Hide resolved
@AuHau AuHau marked this pull request as ready for review March 18, 2021 07:03
@AuHau AuHau requested a review from a team March 18, 2021 07:03
src/modules/file.ts Outdated Show resolved Hide resolved

return true
throw e
Copy link
Member

Choose a reason for hiding this comment

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

I find this a bit strange that a type guard function may throw an exception. Can you describe what is the rationale here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, generally speaking, you always want to catch only those errors that you know you can handle, which in this case is TypeError coming from makeHexString as it signals non valid hex string. Because if you will just "catch all" you can catch an error that is unrelated and the problem causing it might be silenced and then very hard to find.

If I would want to be more thorough I would actually check that it is coming from makeHexString (for example with error codes) as there might be more points on the way which could throw TypeError, but I found that overkill.

I agree that it might not be that much necessary in this case, but I find it generally a best practice to follow this style to prevent possible future problems.

Copy link
Member

Choose a reason for hiding this comment

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

In general this is a good practice, I agree.

However my assumption would be that a type guard is a pure function that returns true or false and it's written in a way that it can positively decide if the input is of a certain type. Every other cases would be false (including potential errors) because we already checked the positive cases.

src/utils/hex.ts Outdated Show resolved Hide resolved
src/utils/hex.ts Outdated Show resolved Hide resolved
Copy link
Member

@nugaon nugaon left a comment

Choose a reason for hiding this comment

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

NIce work!

I made some comments which is related to ticket #143 where type problems has been stated and as I see this PR also brings in this flaw. We must work on that refactoring after this PR merge.

src/utils/hex.ts Outdated Show resolved Hide resolved
src/utils/hex.ts Outdated Show resolved Hide resolved
src/bee-debug.ts Outdated Show resolved Hide resolved
src/types/index.ts Show resolved Hide resolved
AuHau and others added 2 commits March 19, 2021 12:02
Co-authored-by: nugaon <50576770+nugaon@users.noreply.github.com>
@AuHau AuHau merged commit 53a2c25 into master Mar 19, 2021
@AuHau AuHau deleted the feat/hex-string branch March 19, 2021 13:11
@bee-worker bee-worker mentioned this pull request Mar 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants