Skip to content

Conversation

@icppWorld
Copy link

Fix for #4

I included an additional test in tests/test_candid.py, test_principal_decode_2 that demonstrates the bug & the fix.

@eliezhao eliezhao requested a review from Copilot November 20, 2025 08:10
Copilot finished reviewing on behalf of eliezhao November 20, 2025 08:16
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +862 to +866
res = safeReadByte(b)
if leb128.u.decode(res) != 1:
raise ValueError("Cannot decode principal")
length = leb128uDecode(b)
raw = safeRead(b, length)
if raw == b"\x00":
return P.from_str("aaaaa-aa")
return P.from_hex(raw.hex())
return P.from_hex(safeRead(b, length).hex())
Copy link

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

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

The updated decodeValue method expects a flag byte (value 1) before the length, but the corresponding encodeValue method (lines 844-855) does not write this flag byte. This creates an encode/decode incompatibility.

The current encoder produces:

  • For 'aaaaa-aa': "0100" (length=1, data=0x00)
  • For a 29-byte principal: "1d" + 29 bytes (length=29, data=29 bytes)

The new decoder interprets:

  • For 'aaaaa-aa': "0100" as flag=1 ✓, length=0, data= → this works by coincidence
  • For a 29-byte principal: "1d..." as flag=29 ✗ → this fails the check on line 863

The encoder should be updated to write a flag byte:

def encodeValue(self, val) -> bytes:
    if isinstance(val, str):
        buf = P.from_str(val).bytes
    elif isinstance(val, bytes):
        buf = val
    else:
        raise ValueError("Principal should be string or bytes.")
    flag = b"\x01"
    length = leb128.u.encode(len(buf))
    return flag + length + buf

Note: This would change the encoded format and break compatibility with data encoded by the old encoder. The test_principal_encode test would also need updating.

Copilot uses AI. Check for mistakes.
Comment on lines +863 to +864
if leb128.u.decode(res) != 1:
raise ValueError("Cannot decode principal")
Copy link

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

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

The error message "Cannot decode principal" doesn't indicate what went wrong. Consider making it more descriptive to help with debugging:

raise ValueError(f"Invalid principal flag byte: expected 1, got {leb128.u.decode(res)}")

This would make it clearer when the decoder encounters incorrectly formatted principal data.

Suggested change
if leb128.u.decode(res) != 1:
raise ValueError("Cannot decode principal")
val = leb128.u.decode(res)
if val != 1:
raise ValueError(f"Invalid principal flag byte: expected 1, got {val}")

Copilot uses AI. Check for mistakes.
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.

1 participant