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

remove special case for G1Element in SExp.to() #134

Merged
merged 1 commit into from
Aug 16, 2023
Merged

Conversation

arvidn
Copy link
Contributor

@arvidn arvidn commented Aug 16, 2023

Instead support any type that can convert to bytes.

@coveralls-official
Copy link

coveralls-official bot commented Aug 16, 2023

Pull Request Test Coverage Report for Build 5877680554

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 95.345%

Totals Coverage Status
Change from base Build 4176492396: 0.0%
Covered Lines: 916
Relevant Lines: 951

💛 - Coveralls

Copy link
Contributor

@richardkiss richardkiss left a comment

Choose a reason for hiding this comment

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

More general and better. My only concern is that the type signature for convert_atom_to_bytes no longer reflects that G1Element is safe, so may cause mypy problems. This isn't easy to fix unless we ignore py3.7, which is EOL June, 2023, so maybe it's okay. ChatGPT says we can do something like this:

from typing import Protocol, runtime_checkable

@runtime_checkable
class CanBeCastToBytes(Protocol):
    def __bytes__(self) -> bytes: ...

but it only works in 3.8+.

@arvidn
Copy link
Contributor Author

arvidn commented Aug 16, 2023

yeah, as far as I can tell, in all situations we use this in chia-blockchain right now, the types of the list is erased already anyway. So in practice it's not a regression (yet). I suppose we would need to export a protocol that we could use in chia-blockchain as well, as the member of our List[x]

@arvidn arvidn merged commit 0b20c18 into main Aug 16, 2023
18 of 19 checks passed
@arvidn arvidn deleted the g1-element-special-case branch August 16, 2023 23:21
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.

2 participants