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: stabilize NEP 492 #9658

Merged
merged 5 commits into from
Oct 10, 2023
Merged

feat: stabilize NEP 492 #9658

merged 5 commits into from
Oct 10, 2023

Conversation

bowenwang1996
Copy link
Collaborator

@bowenwang1996 bowenwang1996 commented Oct 10, 2023

Stabilize near/NEPs#492. Restrict the creation of non-implicit top-level account that are longer than 32 bytes. Only the registrar account can create them. More context on the proposal:

@bowenwang1996 bowenwang1996 requested a review from a team as a code owner October 10, 2023 01:17
@bowenwang1996
Copy link
Collaborator Author

@akhi3030 do you know why I couldn't request review from @jakmeier ? Is it because of the github organization setup?

@akhi3030
Copy link
Collaborator

@akhi3030 do you know why I couldn't request review from @jakmeier ? Is it because of the github organization setup?

Hmm, it is weird indeed. I'll read some github docs to figure it out.

Copy link
Contributor

@jakmeier jakmeier left a comment

Choose a reason for hiding this comment

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

LGTM. Left one comment regarding the changelog entry.

As a general note, when stabilizing a protocol feature, the PR itself is usually very simple but it is nevertheless a crucial change. Therefore, I love it when the PR author describes what testing has ben done and why it's ready to be stabilized. It usually only takes 30s to write it in the commit message but potentially saves the reviewers much more time.
Of course, in this case, for me, I already have all the context. But adding this information makes it more accessible for anybody else.

For example:

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@akhi3030 akhi3030 left a comment

Choose a reason for hiding this comment

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

Approving to unblock.

@akhi3030
Copy link
Collaborator

@akhi3030 do you know why I couldn't request review from @jakmeier ? Is it because of the github organization setup?

Looking at https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/requesting-a-pull-request-review, I see the following:

"""
Organization members with write access can also assign a pull request review to any person or team with read access to a repository.
"""

So the simple answer is that @ jakmeier does not have read access to the repo. Not sure if github allows giving the world read access. It does not appear that this is possible. https://github.com/near/nearcore/settings/access?query= shows who has what kind of access to the repo currently. I will start an internal thread to discuss making changes here.

@jakmeier
Copy link
Contributor

For what it's worth, tagging me in a comment will do the same for me as adding me as reviewer. Both will show up as GH notification and I will take a look.

@bowenwang1996 bowenwang1996 added this pull request to the merge queue Oct 10, 2023
Merged via the queue into master with commit 3872f67 Oct 10, 2023
@bowenwang1996 bowenwang1996 deleted the stabilize-restrict-tla branch October 10, 2023 18:51
nikurt pushed a commit that referenced this pull request Oct 11, 2023
Stabilize near/NEPs#492. Restrict the creation
of non-implicit top-level account that are longer than 32 bytes. Only
the registrar account can create them. More context on the proposal:

- The NEP has been officially approved in September
(near/NEPs#492 (comment)).
- #9589 implements the feature and includes tests checking that TLA can
no longer be created in the new protocol version.
marcelo-gonzalez added a commit to marcelo-gonzalez/nearcore that referenced this pull request Nov 17, 2023
near#9658 stabilized a protocol feature
that restricts creating top level accounts unless you're the registrar,
and in this test we were trying to create one. change it to be a subaccount
of the tx signer
github-merge-queue bot pushed a commit that referenced this pull request Nov 17, 2023
#9658 stabilized a protocol feature
that restricts creating top level accounts unless you're the registrar,
and in this test we were trying to create one. change it to be a
subaccount of the tx signer
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.

3 participants