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

Make smoke tests check if CBOR tag nums are reserved #436

Merged
merged 1 commit into from
Aug 27, 2024

Conversation

fxamacker
Copy link
Member

This PR checks if CBOR tag number is reserved by atree for internal use before smoke tests use it to encode Cadence values as elements in atree containers.

As of Aug 15, 2024:

  • Atree reserves CBOR tag numbers [240, 255] for internal use.
  • Smoke tests reserves CBOR tag numbers [161, 239] for encoding elements in atree containers.

  • Targeted PR against main branch
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work
  • Code follows the standards mentioned here
  • Updated relevant documentation
  • Re-reviewed Files changed in the Github PR explorer
  • Added appropriate labels

This commit checks if CBOR tag number is reserved by atree
for internal use before smoke tests use it to encode Cadence
values as elements in atree containers.

As of Aug 15, 2024:
- Atree reserves CBOR tag numbers [240, 255] for internal use.
- Smoke tests reserves CBOR tag numbers [161, 239] for encoding
  elements in atree containers.
@fxamacker
Copy link
Member Author

@ramtinms PTAL 🙏 the equivalent in Cadence repo for checking tags before use was approved by Bastian in

@fxamacker
Copy link
Member Author

@turbolent PTAL 🙏

Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

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

Nice!

Sorry about the late review! I had confused this with the other PR, #434.

@fxamacker
Copy link
Member Author

Nice!

Sorry about the late review! I had confused this with the other PR, #434.

No worries!

@fxamacker fxamacker merged commit 37b6546 into main Aug 27, 2024
6 checks passed
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.

2 participants