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

bugfix: repro and fix for issue #4267 (reject too long principals in Principal.fromBlob and actor literals) #4268

Merged
merged 8 commits into from
Nov 7, 2023

Conversation

crusso
Copy link
Contributor

@crusso crusso commented Nov 6, 2023

Fixes issues #4267:

  • BREAKING CHANGE (Minor): values of type Principal are now constrained to contain
    at most 29 bytes, matching the IC's notion of principal (bugfix: repro and fix for issue #4267 (reject too long principals in Principal.fromBlob and actor literals) #4268).

    In particular:

    • An actor import will be statically rejected if the binary representation of the (aliased) textually encoded
      principal contains strictly more than 29 bytes.

    • Principal.fromBlob(b) will trap if b contains strictly more than 29 bytes.

    • The actor literal, actor <exp>, will trap if the binary representation of
      of the textually encoded principal <exp> contains strictly more than 29 bytes.

Copy link

github-actions bot commented Nov 6, 2023

Comparing from 7fbe013 to f70b545:
In terms of gas, 1 tests regressed, 2 tests improved and the mean change is -1.6%.
In terms of size, 5 tests regressed and the mean change is +0.0%.

@crusso crusso changed the title repro for issue #4267 (reject too long principals) bugfix: repro and fix for issue #4267 (reject too long principals in Principal.fromBlob and actor literals) Nov 6, 2023
@crusso
Copy link
Contributor Author

crusso commented Nov 6, 2023

Part of me thinks it would have been cleaner just to remove the candid length check on deserialization and rely on the IC checking principal lengths... that would avoid changing the candid spec too....

@crusso crusso marked this pull request as ready for review November 6, 2023 20:41
Copy link
Collaborator

@nomeata nomeata left a comment

Choose a reason for hiding this comment

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

LGTM

@crusso crusso added the automerge-squash When ready, merge (using squash) label Nov 7, 2023
@mergify mergify bot merged commit d80da65 into master Nov 7, 2023
9 checks passed
@mergify mergify bot removed the automerge-squash When ready, merge (using squash) label Nov 7, 2023
@mergify mergify bot deleted the claudio/issue-4267 branch November 7, 2023 13:48
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