Expand Bech32 usage #473
Expand Bech32 usage #473
Conversation
…ing/decoding support
…ernance action IDs
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #473 +/- ##
==========================================
- Coverage 90.65% 90.61% -0.05%
==========================================
Files 33 33
Lines 5007 5155 +148
Branches 758 782 +24
==========================================
+ Hits 4539 4671 +132
- Misses 297 305 +8
- Partials 171 179 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
cffls
left a comment
cffls
left a comment
There was a problem hiding this comment.
Thanks very much for adding this feature! Overall looks pretty good in terms of implementation. Only a few comments regarding the interfaces.
pycardano/certificate.py
Outdated
| ) | ||
| """The credential associated with this DRep, if applicable""" | ||
|
|
||
| def id(self, id_format: IdFormat = IdFormat.CIP129) -> str: |
There was a problem hiding this comment.
I would move id_format to an internal field of DRep. Doing so would lead to a better alignment with how Address type work today.
For example, an address has the following usage pattern:
>>> addr = Address.from_primitive("addr_test1vr2p8st5t5cxqglyjky7vk98k7jtfhdpvhl4e97cezuhn0cqcexl7")
addr
addr_test1vr2p8st5t5cxqglyjky7vk98k7jtfhdpvhl4e97cezuhn0cqcexl7
>>> bytes(addr)
b'`\xd4\x13\xc1t]0`#\xe4\x95\x89\xe6X\xa7\xb7\xa4\xb4\xdd\xa1e\xff\\\x97\xd8\xc8\xb9y\xbf'
>>> bytes(addr).hex()
'60d413c1745d306023e49589e658a7b7a4b4dda165ff5c97d8c8b979bf'
>>> str(addr)
'addr_test1vr2p8st5t5cxqglyjky7vk98k7jtfhdpvhl4e97cezuhn0cqcexl7'There are two essential calls: str(addr) and bytes(addr).
str(addr) is implemented by __repr__, and bytes(addr) is implemented by __bytes__.
Similarly, it would be great if DRep could stick to this usage pattern.
Something like this:
>>> drep = DRep(..., IdFormat.CIP105)
>>> bytes(drep).hex()
'4828a2dadba97ca9fd0cdc99975899470c219bdc0d828cfa6ddf6d69'
>>> str(drep)
'drep1fq529kkm4972nlgvmjvewkyeguxzrx7upkpge7ndmakkjnstaxx'
pycardano/certificate.py
Outdated
| """ | ||
| return self.encode(id_format) | ||
|
|
||
| def id_hex(self, id_format: IdFormat = IdFormat.CIP129) -> str: |
There was a problem hiding this comment.
As mentioned above, this seems a bit redundant, as user could simply call bytes(drep).hex() instead.
pycardano/governance.py
Outdated
| def id(self) -> str: | ||
| """ | ||
| Get the governance action ID in Bech32 format. | ||
| """ | ||
| return self.encode() | ||
|
|
||
| def id_hex(self) -> str: | ||
| """ | ||
| Get the governance action ID in hexadecimal format. | ||
| """ | ||
| return bytes(self).hex() |
There was a problem hiding this comment.
Similar to DRep, let's use the same interface (bytes and str) to get the representations.
|
Thanks for the feedback. I will work on this today. |
cffls
left a comment
cffls
left a comment
There was a problem hiding this comment.
Looks good to me. Thank you for making this happen!
2 participants
This pull request introduces enhanced support for Bech32 encoding/decoding for representing DRep credentials, committee credentials, pool operators, and governance action IDs. These changes also include support for both CIP105 and CIP129 variations.
Governance and DRep credential improvements:
GovernanceCredentialandGovernanceKeyType, with support for Bech32 encoding/decoding, hex representation, and credential type identification. Also updatedDRepCredentialto inherit fromGovernanceCredential. [1] [2]DRepclass with methods for encoding/decoding DRep IDs in Bech32 and hex formats, and improved error handling. [1] [2]Governance action and committee credential enhancements:
CommitteeHotCredentialand updatedCommitteeColdCredentialto inherit fromGovernanceCredential, supporting Bech32 encoding. [1] [2]GovActionId, allowing Bech32 and hex representations for governance action IDs.Pool operator improvements:
PoolOperatorclass with Bech32 encoding/decoding, hex representation, and primitive conversion methods for pool key hashes. [1] [2]Supporting changes:
CIP129_PAYLOAD_SIZEto support new credential formats. [1] [2]