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

feat: Increase namespace size #171

Merged
merged 4 commits into from
Aug 11, 2023

Conversation

rach-id
Copy link
Member

@rach-id rach-id commented Aug 8, 2023

Overview

Closes #148

Checklist

  • New and updated code has appropriate documentation
  • New and updated code has new and/or updated testing
  • Required CI checks are passing
  • Visual proof for any user facing features like CLI or documentation updates
  • Linked issues closed with keywords

@rach-id rach-id requested a review from adlerjohn August 8, 2023 00:03
@rach-id rach-id self-assigned this Aug 8, 2023
Comment on lines 18 to +20
/// @dev Parity share namespace ID
NamespaceID internal constant PARITY_SHARE_NAMESPACE_ID = NamespaceID.wrap(0xFFFFFFFFFFFFFFFF);
NamespaceID internal constant PARITY_SHARE_NAMESPACE_ID =
NamespaceID.wrap(0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF);
Copy link
Contributor

Choose a reason for hiding this comment

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

[optional] in a separate PR it may make sense to rename namespace ID to namespace for similar reasons to celestiaorg/nmt#206

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

@adlerjohn adlerjohn left a comment

Choose a reason for hiding this comment

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

only one thought

@@ -1,7 +1,7 @@
// SPDX-License-Identifier: Apache-2.0
pragma solidity ^0.8.19;

type NamespaceID is bytes8;
type NamespaceID is bytes29;
Copy link
Member

Choose a reason for hiding this comment

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

Hmmm. Should this instead be a struct of a bytes1 and bytes28 https://github.com/celestiaorg/celestia-app/blob/3e6d22cfde678efac4f7c078228e9f294a7a9b8d/specs/src/specs/namespace.md#overview? The actual namespace ID isn't just one field. There isn't any code in the QGB proper that cares, but perhaps downstream contracts might care (e.g. to ensure their data was posted to the correct namespace).

Doing that would increase costs though.

Copy link
Member Author

@rach-id rach-id Aug 10, 2023

Choose a reason for hiding this comment

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

Does it make sense to handle this in a separate PR? I can create an issue for it and then make the change along with this one #172

@rach-id rach-id requested a review from adlerjohn August 10, 2023 22:21
@rach-id rach-id merged commit d58f72d into celestiaorg:master Aug 11, 2023
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.

Increase namespace size
3 participants