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

[ACP-77] Adjust naming of txn types due to renaming of Avalanche Subnets -> Avalanche L1s #154

Open
wants to merge 45 commits into
base: main
Choose a base branch
from

Conversation

meaghanfitzgerald
Copy link
Contributor

@meaghanfitzgerald meaghanfitzgerald commented Oct 15, 2024

As stated in this Avalanche Academy Guide: Avalanche Subnets have been rebranded as Avalanche L1s.

To avoid confusion, the spec has been revised to account for this, including the following changes:

SubnetValidator (struct) -> L1Validator
ConvertSubnetTx -> ConvertSubnetToL1Tx
RegisterSubnetValidatorTx -> RegisterL1ValidatorTx
RegisterSubnetValidatorMessage -> RegisterL1ValidatorMessage
SetSubnetValidatorWeightTx -> SetL1ValidatorWeightTx
DisableValidatorTx -> DisableL1ValidatorTx
SubnetValidatorRegistrationMessage -> L1ValidatorRegistrationMessage
IncreaseBalanceTx -> IncreaseL1ValidatorBalanceTx 
SubnetValidatorWeightMessage -> L1ValidatorWeightMessage
SubnetConversionMessage -> L1ConversionMessage

To Review:

ReadMe Preview

@@ -564,7 +569,7 @@ The continuous fee mechanism outlined above does not apply to inactive Subnet Va

### Should they still be called Subnets?

Through this ACP, Subnets have far more sovereignty than they did before. The Avalanche Community could take this opportunity to re-brand Subnets.
Going forward, Subnets should be referred to as L1s. Unlike layer 2 blockchains, layer 1 blockchains operate independently without relying on other systems for their core functions. They manage their own consensus mechanisms, transaction processing, and security protocols directly within their own networks. Subnets have always operated this way. With the added functionality introduced in this ACP, they are clearly standalone networks, or L1s.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it could be worth mentioning the fact that L1 validator sets creating using the new ACP-77 flow will not be a subset of the primary network validator set, making it technically incorrect to call them Subnets.

This is neither here or there, and I see the note in the abstract also, but feel like this explanation belongs more in "motivation" since it is no longer an open question IMO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

my original pr included it as the first part of the motivation, but @dhrubabasu disagrees. I've linked it in line 19 but am open to moving it

Copy link
Contributor

@michaelkaplan13 michaelkaplan13 left a comment

Choose a reason for hiding this comment

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

🙏 The new transaction names make much more sense to me.

Only top-level comment is a suggestion to put the explanation of the L1 name (and the difference from a Subnet), and then updating all the references to the new L1s.

The technical difference between them removes the ambiguity about which would should be used when, and in my opinion makes it far clearer to comprehend and conform to.

ACPs/77-reinventing-subnets/README.md Outdated Show resolved Hide resolved
ACPs/77-reinventing-subnets/README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@StephenButtolph StephenButtolph left a comment

Choose a reason for hiding this comment

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

There are more instances where Subnet seems to be incorrectly used now. But I don't think there is much value in me marking all of them.

Seems like we should be consistent

ACPs/77-reinventing-subnets/README.md Outdated Show resolved Hide resolved
ACPs/77-reinventing-subnets/README.md Outdated Show resolved Hide resolved
ACPs/77-reinventing-subnets/README.md Outdated Show resolved Hide resolved
ACPs/77-reinventing-subnets/README.md Outdated Show resolved Hide resolved
ACPs/77-reinventing-subnets/README.md Outdated Show resolved Hide resolved
ACPs/77-reinventing-subnets/README.md Outdated Show resolved Hide resolved
ACPs/77-reinventing-subnets/README.md Outdated Show resolved Hide resolved
ACPs/77-reinventing-subnets/README.md Outdated Show resolved Hide resolved
ACPs/77-reinventing-subnets/README.md Outdated Show resolved Hide resolved
ACPs/77-reinventing-subnets/README.md Outdated Show resolved Hide resolved
@michaelkaplan13
Copy link
Contributor

🙏 The new transaction names make much more sense to me.

Only top-level comment is a suggestion to put the explanation of the L1 name (and the difference from a Subnet), and then updating all the references to the new L1s.

The technical difference between them removes the ambiguity about which would should be used when, and in my opinion makes it far clearer to comprehend and conform to.

@dhrubabasu do you agree here? It definitely results in a larger change to the ACP readme in terms of lines changed, but I think it makes it far clearer and less ambiguous which term should be used when going forward. It makes the most sense to update now in this ACP since it is what introduces the new terminology and will be referenced going forward.

@dhrubabasu
Copy link
Contributor

dhrubabasu commented Oct 21, 2024

🙏 The new transaction names make much more sense to me.
Only top-level comment is a suggestion to put the explanation of the L1 name (and the difference from a Subnet), and then updating all the references to the new L1s.
The technical difference between them removes the ambiguity about which would should be used when, and in my opinion makes it far clearer to comprehend and conform to.

@dhrubabasu do you agree here? It definitely results in a larger change to the ACP readme in terms of lines changed, but I think it makes it far clearer and less ambiguous which term should be used when going forward. It makes the most sense to update now in this ACP since it is what introduces the new terminology and will be referenced going forward.

I agree. Let's move up the explanation to the Motivation section and put "No open questions." in the Open Questions section. We may want to call out that this rename was done with overwhelming community support in the Motivation section as well

I didn't want it moved up in the prior PR because there wasn't a clear delineation between Subnets and L1s in the body of the ACP itself. Like @StephenButtolph pointed out, we should be consistent in the ACP to use Subnet and L1 per the delineation we're making. I don't mind the size of the change as long as the ACP becomes clearer and less ambiguous like this PR is doing with the rename (h/t @meaghanfitzgerald for spearheading this effort 🏆)

Copy link
Contributor

@michaelkaplan13 michaelkaplan13 left a comment

Choose a reason for hiding this comment

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

Haven't done a full pass, but think there are a few places where the term "subnet" is still used in reference to what is actually an L1.

ACPs/77-reinventing-subnets/README.md Outdated Show resolved Hide resolved
ACPs/77-reinventing-subnets/README.md Outdated Show resolved Hide resolved
ACPs/77-reinventing-subnets/README.md Outdated Show resolved Hide resolved
ACPs/77-reinventing-subnets/README.md Outdated Show resolved Hide resolved
ACPs/77-reinventing-subnets/README.md Outdated Show resolved Hide resolved
ACPs/77-reinventing-subnets/README.md Outdated Show resolved Hide resolved
ACPs/77-reinventing-subnets/README.md Outdated Show resolved Hide resolved
ACPs/77-reinventing-subnets/README.md Outdated Show resolved Hide resolved
ACPs/77-reinventing-subnets/README.md Outdated Show resolved Hide resolved
ACPs/77-reinventing-subnets/README.md Show resolved Hide resolved
Copy link
Contributor

@dhrubabasu dhrubabasu left a comment

Choose a reason for hiding this comment

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

Great work going through and editing the ACP to delineate Subnets and L1s! A lot of these comments are more minor but thought it'd be better to be exhaustive so we can merge this in soon and then get #156 in)

I think it should be good to merge after this

ACPs/77-reinventing-subnets/README.md Outdated Show resolved Hide resolved
ACPs/77-reinventing-subnets/README.md Outdated Show resolved Hide resolved
ACPs/77-reinventing-subnets/README.md Outdated Show resolved Hide resolved
ACPs/77-reinventing-subnets/README.md Show resolved Hide resolved
ACPs/77-reinventing-subnets/README.md Show resolved Hide resolved
ACPs/77-reinventing-subnets/README.md Outdated Show resolved Hide resolved
ACPs/77-reinventing-subnets/README.md Outdated Show resolved Hide resolved
ACPs/77-reinventing-subnets/README.md Outdated Show resolved Hide resolved
ACPs/77-reinventing-subnets/README.md Outdated Show resolved Hide resolved
ACPs/77-reinventing-subnets/README.md Show resolved Hide resolved
@meaghanfitzgerald
Copy link
Contributor Author

Awaiting approval @dhrubabasu

Copy link
Contributor

@dhrubabasu dhrubabasu left a comment

Choose a reason for hiding this comment

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

few more nits

ACPs/77-reinventing-subnets/README.md Outdated Show resolved Hide resolved
ACPs/77-reinventing-subnets/README.md Outdated Show resolved Hide resolved
ACPs/77-reinventing-subnets/README.md Outdated Show resolved Hide resolved
ACPs/77-reinventing-subnets/README.md Outdated Show resolved Hide resolved
ACPs/77-reinventing-subnets/README.md Outdated Show resolved Hide resolved
ACPs/77-reinventing-subnets/README.md Outdated Show resolved Hide resolved
ACPs/77-reinventing-subnets/README.md Outdated Show resolved Hide resolved
ACPs/77-reinventing-subnets/README.md Outdated Show resolved Hide resolved
ACPs/77-reinventing-subnets/README.md Outdated Show resolved Hide resolved
@meaghanfitzgerald
Copy link
Contributor Author

anything else? @dhrubabasu

Copy link
Contributor

@dhrubabasu dhrubabasu left a comment

Choose a reason for hiding this comment

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

LGTM after the last comment is addressed!

Could we rename this PR to "[ACP-77] Update with Layer 1 terminology" or something similar? We're not renaming Subnets to L1s in the latest form of this PR

Address []byte `json:"address"`
// Initial pay-as-you-go validators for the Subnet
Validators []SubnetValidator `json:"validators"`
// Initial set of validators for the L1
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be modified to say something similar to continuous-fee-paying. We wouldn't be supplanting the existing validator set with the ones defined in this array.

Not married to "continuous-fee-paying," feel free to modify

Suggested change
// Initial set of validators for the L1
// Initial continuous-fee-paying validators for the L1

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.

4 participants