-
Notifications
You must be signed in to change notification settings - Fork 1.1k
SPO Generating wallet keys: add stake verification key to payment address #1483
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
base: staging
Are you sure you want to change the base?
Conversation
|
@DaveBelv is attempting to deploy a commit to the Cardano Foundation Team on Vercel. A member of the Team first needs to authorize it. |
| ``` | ||
| cardano-cli address build \ | ||
| --payment-verification-key-file payment.vkey \ | ||
| --stake-verification-key-file stake.vkey \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reason for the change? If including stake key, this will no longer be just a "payment address".
This particular address does not need to stake directly, it is used to pay for deposits and transactions, the funding of stake account is independent activity, which is especially true when [unfortunately] people are using multi-owner accounts wherein pool operator may not be contributing via funds - on testnets, this address is recipient of the faucet funds (unless there is a reference elsewhere - if so, please add that as context)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The context of the paragraph might need to be touched up instead if this was the intended purpose. The explaining paragraph for this line of code is:
generate a wallet address for the payment key `payment.vkey` which will delegate to the stake address `stake.vkey`
Which is used going forward in the file i.e. there is no additional instructions to then craft a transaction to the payment address which is delegated thereby enabling the active pledge on the pool - it just ends with success once the pool has been registered.
When I read that as a part of setting up the node I was under the impression there was no additional work and the ADA in the payment address would be the enabling the active pledge (which isn't the case).
Did you want the changes instead to reflect this? As in tidy up the language in the explaining line above this code to reflect this as the payment and then add in the additional section at the end when you have registered the stake pool to then transfer the ADA to enable the active pledge?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rdlrt indeed the English description of what is being done is accurate only when incorporating the stake key into the payment address as suggested.
From my own interaction with other SPOs I would say this configuration (one do-it-all payment address: also constituting the pledge and staked to the pool) is probably used by more new operators (but maybe not by more pools: since, as you say, operational requirements will often be different).
So I would support this change although I would generally recuse myself from discussions about the best configuration for pools on the testnets. I've never been behind the convention of our SPO documentation addressing testnets rather than Mainnet: where I've always silently believed it's best just for operators to create a pool and then re-register it on Mainnet if & when it evolves.
Therefore I'll follow along with other reviewers' inclinations about this... but either the code or the English do need to be modified so they match.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fair, I think the flow loses the context when it assumes payment.addr is linked to stake account here without clarifying how it was created.
Overall the language is a bit inconsistent on portal:
- Payment address would typically be associated to payment key hash (which is also previously referred to as
Enterpriseaddress). - Many guides call the addresses with linked stake key as base address.
- I believe the intent here was to get initial address populated with faucet funds, and from there on - go and create the actual wallet keys to be used for stake pool operations, which - IMO - would be a better step than directly using single address for everything. If so, we should potentially update
payment.addrreference onregister-stake-pool.mdand replace it withbase.addr, we can include a section in this page to create base.addr as well (the source of funds for existing transaction definitions would remain the same).
Ofcourse, that goes beyond the original scope of what this PR was meant to address, so happy to just go with approval for now - if we'd like to tackle this later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can treat that as an open question @rdlrt . The intent of the PR was to clarify the (what I'm hoping I get correct) base.addr when node operators are using the instructions to create nodes & stake pools.
If this way (updating the wording across the docs and adding a new section for this) is the preferred option to handle base vs payment as different addresses then it would be the solution to the problem this PR is attempting to address.
Let me know which way you would prefer this to be addressed and happy to implement it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My vote would be keep wording here referring to payment address and create a base address, while using base address as reference on register-stake-pool.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to edit the PR to reflect that. I think it will also help understanding of the stake pool (primarily the active pledge) and the difference between the accounts.
I'll also add a note that other guides (atleast coincashew) might use the base address over the payment here (or when the guide creates the base) to help with the overall understanding across the user base.
@rdlrt can you please mark as work required just to be clear we are heading down this path. Thanks!
|
@katomm @aldo555 @rdlrt does anyone know how/why Vercel was involved in the processing of this PR? Has the GitHub CI changed to include this somehow? 🤔 |
rphair
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving as first order approximation suggested in #1483 (comment).
rdlrt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per discussions, aligned to update wordings to use payment addresses for payment keys (as now) and create a small heading for creating base address.
Additionally, the reference needs to be updated on successive pages for stake pool registrations
Consensus favours changing this across multiple pages.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@rphair We are doing some testing with vercel but currently there are no production deployments there so don't mind it. |
👋 Hello there! Welcome. Please follow the steps below to tell us about your contribution.
Checklist
<-- Please fill the boxes with [x] before submitting a pull request -->
yarn buildafter adding my changes without getting any errors.yarn.lock(or have removed these changes).Updating documentation or Bugfix
The command for generating the payment.addr needs the stake.vkey otherwise generated address will not delegate to the stake address.