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

docs: (cli) minor updates to deploy-a-program.md #34307

Merged

Conversation

norwnd
Copy link
Contributor

@norwnd norwnd commented Dec 2, 2023

Problem

Improving documentation for program management.

Summary of Changes

This is a spin-off from #33860 to extend program management docs with some extra details. See rendered version.

@mergify mergify bot added community Community contribution need:merge-assist labels Dec 2, 2023
@mergify mergify bot requested a review from a team December 2, 2023 17:38
Comment on lines 235 to 239
In case you want to set "new upgrade authority" to a signer that you only have a pubkey of (meaning, you don't have access
to its private key) - which is useful for things like [upgrading program using offline signer as authority](deploy-a-program.md#upgrading-program-using-offline-signer-as-authority) -
you need to use `--skip-new-upgrade-authority-signer-check` option to inform `solana program set-upgrade-authority`
command of your intentions (because otherwise it assumes you have access to that singer's private key and checks for
that, to ensure you don't accidentally supply "new upgrade authority" you don't have control over):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

[upgrading program using offline signer as authority](deploy-a-program.md#upgrading-program-using-offline-signer-as-authority) section will be available once #33860 is merged.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove the reference to the new section. This PR will go in first, which means that the docs would point to nothing for some time. For consistency, let's add the reference when it goes in

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, can add it later, removed in a229657

Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

Thanks for cleaning a lot of these bits up! This should be ready to go soon

Comment on lines 112 to 115
Note that program accounts are required to be
[rent-exempt](developing/programming-model/accounts.md#rent-exemption), and the
`max-len` is fixed after initial deployment, so any SOL in the program accounts
is locked up permanently.
`max-len` **cannot be changed** after initial deployment, yet any SOL in the program accounts
is locked up permanently and cannot be reclaimed.
Copy link
Contributor

Choose a reason for hiding this comment

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

Neither of these are currently true, since you can close a program to reclaim the lamports and you can extend a program to use more data. Do you mind removing this whole paragraph?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, removed in a229657

Comment on lines 51 to 53
You can typically find a matching program keypair file is in the same directory
as the program's shared object, and named <PROGRAM_NAME>-keypair.json. Matching
program keypairs are generated automatically by the program build tools:
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this change for? With the change in wording, it makes it seem like the keypair isn't always generated. Is that the case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess I was trying to point out that program keypairs aren't there until you compile your program's source code. Re-reading it now - can't say I succeeded : )

Perhaps we'd want to put Matching program keypairs are generated ... sentence first to make it abundantly clear, but for now I've reverted this change in a229657

that the `dump` command dumps the entire data space, which means the output file
will have trailing zeros after the shared object's data up to `max_len`.
might have trailing zeros after the shared object's data up to `max_len`.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I think this should stay will, since it will always have trailing zeros after the shared object's data up to max_len

Copy link
Contributor Author

@norwnd norwnd Dec 7, 2023

Choose a reason for hiding this comment

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

Was trying to be precise (I think there shouldn't be trailing zeros if shared object's data == max_len ?), anyway reverted in a229657


```bash
solana program write-buffer <PROGRAM_FILEPATH>
```

Buffer accounts support authorities like program accounts:
Buffer accounts support different authorities (including offline signer and program account signer):
Copy link
Contributor

Choose a reason for hiding this comment

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

The original sentence means that buffers have an authority, similar to program accounts. With the change, it makes it sound like a buffer has multiple authorities, which is not the case. So let's revert the change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha, reverted in a229657

Comment on lines 311 to 312
Note, the buffer's authority must match the program's upgrade authority. Also, upon successful deploy
buffer accounts contents are copied into program accounts and are erased from blockchain.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
Note, the buffer's authority must match the program's upgrade authority. Also, upon successful deploy
buffer accounts contents are copied into program accounts and are erased from blockchain.
Note, the buffer's authority must match the program's upgrade authority.
During deployment, the buffer account's contents are copied into the program-data account and
the buffer account is set to zero. The lamports from the buffer account are refunded to a spill account.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Applied in a229657

@@ -275,13 +285,14 @@ $ sha256sum extended.so dump.so
Instead of deploying directly to the program account, the program can be written
to an intermediary buffer account. Intermediary accounts can be useful for things
like multi-entity governed programs where the governing members fist verify the
intermediary buffer contents and then vote to allow an upgrade using it.
intermediary buffer contents and then vote to allow an upgrade using it, or for
[deploying programs with offline signer authority](#deploying-program-with-offline-signer-authority).
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's revert this change and have it in the original PR. It's strange to reference a section that doesn't exist!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, removed in a229657

Comment on lines 235 to 239
In case you want to set "new upgrade authority" to a signer that you only have a pubkey of (meaning, you don't have access
to its private key) - which is useful for things like [upgrading program using offline signer as authority](deploy-a-program.md#upgrading-program-using-offline-signer-as-authority) -
you need to use `--skip-new-upgrade-authority-signer-check` option to inform `solana program set-upgrade-authority`
command of your intentions (because otherwise it assumes you have access to that singer's private key and checks for
that, to ensure you don't accidentally supply "new upgrade authority" you don't have control over):
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove the reference to the new section. This PR will go in first, which means that the docs would point to nothing for some time. For consistency, let's add the reference when it goes in

Comment on lines 235 to 239
In case you want to set "new upgrade authority" to a signer that you only have a pubkey of (meaning, you don't have access
to its private key) - which is useful for things like [upgrading program using offline signer as authority](deploy-a-program.md#upgrading-program-using-offline-signer-as-authority) -
you need to use `--skip-new-upgrade-authority-signer-check` option to inform `solana program set-upgrade-authority`
command of your intentions (because otherwise it assumes you have access to that singer's private key and checks for
that, to ensure you don't accidentally supply "new upgrade authority" you don't have control over):
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: How about we give the precise motivation

Suggested change
In case you want to set "new upgrade authority" to a signer that you only have a pubkey of (meaning, you don't have access
to its private key) - which is useful for things like [upgrading program using offline signer as authority](deploy-a-program.md#upgrading-program-using-offline-signer-as-authority) -
you need to use `--skip-new-upgrade-authority-signer-check` option to inform `solana program set-upgrade-authority`
command of your intentions (because otherwise it assumes you have access to that singer's private key and checks for
that, to ensure you don't accidentally supply "new upgrade authority" you don't have control over):
By default, `set-upgrade-authority` requires a signature from the new authority. This behavior prevents a
developer from giving upgrade authority to a key that they do not have access to. The
`--skip-new-upgrade-authority-signer-check` option relaxes the signer check. This can be useful for situations
where the new upgrade authority is an offline signer or a multisig.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Applied in a229657

@norwnd norwnd requested a review from joncinque December 7, 2023 10:08
Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

One last bit then this is good to go!

Comment on lines 94 to 95
The command looks the same as the deployment command (same keypair file that resides from file directory corresponding
to <PROGRAM_FILEPATH>, this keypair file will be generated once and then reused):
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I don't understand what this is trying to say anymore, and think the previous sentence was clearer. What was the exact problem with the previous language?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops, bad phrasing on my part, I was trying to elaborate that deploy command in "upgrade" mode will also be using that same keypair file that was used during "initial deploy" mode (and that's how it knows what program to upgrade on-chain),

perhaps that's an implementation detail that's not really needed in the docs, removing for now: 6867112 (let me know if you want it added to the docs in some other form)

Copy link
Contributor

Choose a reason for hiding this comment

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

How about we go back to the previous sentence that was there? With this change, there's no more reference to the generated keypair at all

Copy link
Contributor Author

@norwnd norwnd Dec 7, 2023

Choose a reason for hiding this comment

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

Pretty much, the Matching keypair files are generated is a bit unspecific/lacking for my taste (which is why I tried changing it),

perhaps repharase:

Matching keypair files are generated once so that
redeployments will be to the same program address.

into something like ?

Program signer stored in keypair file at <PROGRAM_FILEPATH> is 
generated once (when program is compiled for the very first time) and 
will be used to identify which on-chain program (what Solana address) 
redeployment targets.

Copy link
Contributor

Choose a reason for hiding this comment

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

How about we simplify and explain how the CLI behaves:

If a program id is not provided, the program will be deployed to the default address
at `<PROGRAM_NAME>-keypair.json`. This default keypair is generated during the first
program compilation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

Thanks for fixing these up!

@joncinque
Copy link
Contributor

Merging without the buildkite CI, this is a pure docs change.

@joncinque joncinque merged commit 74c54a7 into solana-labs:master Dec 7, 2023
5 of 6 checks passed
nickfrosty pushed a commit to nickfrosty/solana that referenced this pull request Dec 8, 2023
* docs: (cli) minor updates to deploy-a-program.md

* address review comments

* remove unnecessary impl details from the docs about deploy command upgrade flow

* clarify program redeploy section

---------

Co-authored-by: norwnd <norwnd>
willhickey pushed a commit that referenced this pull request Dec 11, 2023
* feat: moved common docs to  repo

* refactor: removed sidebar items

* refactor: removed unused images

* fix: terminology link

* fix: introduction links

* fix: developing links

* refactor: fixed assorted links

* fix: added back the home index

* refactor: home page links

* refactor: primary links

* fix: links

* fix: updated existing redirects

* feat: added new redirects

* refactor: moved cli index file to cli folder

* feat: turned breadcrumbs on

* feat: auto generated cli sidebar

* refactor: page titles

* feat: added usage and wallets categories

* refactor: moved wallet-guide/cli

* style: page titles

* refactor: renamed file to install

* style: page title

* refactor: relocated file to cli/usage/index.md

* style: page title

* refactor: relocat detailed usage generator for cli commands

* refactor: relocated clie usage files

* refactor: relocated paper wallet file

* refactor: relocated file system wallet doc

* feat: added hardware wallet category

* refactor: relocated hardware wallet overview

* refactor: relocated ledger wallet doc

* style: clie wallet titles

* refactor(revert): relocated cli usage doc

* refactor: relocated to examples

* style: cli examples category title

* style: usage doc title

* refactor: relocated cli intro doc

* style: category title

* refactor: renamed file

* refactor: renamed file

* fix: cli links

* refactor: relocated file

* refactor: relocated files

* fix: more cli links

* refactor: sidebar order

* fix: final cli links?

* refactor: proposals

* refactor: split sidebars

* refactor: removed unused icons

* refactor: relocated file

* refactor: relocated file

* refactor: relocated file

* refactor: relocated file

* feat: added architecture page

* refactor: reloacted filed

* refactor: adjusted header links

* style: sidebar labels

* feat: clusters sidebar details

* style: sidebar label

* refactor: relocate file

* refactor: relocated files

* refactor: relocated files

* refactor: relocated files

* style: validator sidebar

* style: sidebar styles

* refactor: internal links

* style: sidebar order

* fix: internal links

* feat: master sidebar

* refactor: removed unneeded h2

* fix: link redirects

* refactor: relocated pages

* style: runtime links

* refactor: simplified runtime redirects

* fix: internal redirect

* refactor: moved proposals to dropdown

* docs: Removes accounts-on-ramdisk section (#33655)

* RPC: update websocket docs (#33460)

* [rpc]: update websocket docs

* rename rewards to showRewards

* add remaining optional fields for slotsUpdates

* update block subscription showRewards

* Change getHealth to compare optimistically confirmed slots (#33651)

The current getHealth mechanism checks a local accounts hash slot vs.
those of other nodes as specified by --known-validator. This is a
very coarse comparison given that the default for this value is 100
slots. More so, any nodes using a value larger than the default
(ie --incremental-snapshot-interval 500) will likely see getHealth
return status behind at some point.

Change the underlying mechanism of how health is computed. Instead of
using the accounts hash slots published in gossip, use the latest
optimistically confirmed slot from the cluster. Even when a node is
behind, it is able to observe cluster optimistically confirmed by slots
by viewing votes published in gossip.

Thus, the latest cluster optimistically confirmed slot can be compared
against the latest optimistically confirmed bank from replay to
determine health. This new comparison is much more granular, and not
needing to depend on individual known validators is also a plus.

* build(deps): bump @babel/traverse from 7.19.6 to 7.23.2 in /docs (#33726)

Bumps [@babel/traverse](https://github.com/babel/babel/tree/HEAD/packages/babel-traverse) from 7.19.6 to 7.23.2.
- [Release notes](https://github.com/babel/babel/releases)
- [Changelog](https://github.com/babel/babel/blob/main/CHANGELOG.md)
- [Commits](https://github.com/babel/babel/commits/v7.23.2/packages/babel-traverse)

---
updated-dependencies:
- dependency-name: "@babel/traverse"
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* docs: move rpc info to rpc docs (#33723)

docs: link fixes

docs: link fixes

docs: link fixes

* Fix typos in documentation for Secp256k1 native program (#33796)

* docs: outline requirement of stake in order to vote (#33842)

* docs: outline requirement of stake in order to vote

* pr feedback: move stake section up

* chore: fix some typos (#33833)

* fix spelling of "retrieved"
* fix spelling of "should"
* fix spelling of "comparisons"

* docs: updating apt install to apt upgrade (#33920)

* Fix some typo in the documentation (#34058)

Co-authored-by: Andrew Fitzgerald <[email protected]>

* fix: internal links

* refactor: removed rpc api docs

* refactor: removed rpc sidebar

* fix: updated remaining rpc api links

* refactor: removed final rpc /api route

* refactor: removed dangling component files

* refactor: changed copyright

* fix: dangling ordered list

* refactor: wording around solana docs

* feat: home page content

* refactor: updated docs url

* Link to latest version of the off-chain message signing proposal in the docs (#34329)

* docs: (cli) minor updates to deploy-a-program.md (#34307)

* docs: (cli) minor updates to deploy-a-program.md

* address review comments

* remove unnecessary impl details from the docs about deploy command upgrade flow

* clarify program redeploy section

---------

Co-authored-by: norwnd <norwnd>

* refactor: removed GA

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: Brooks <[email protected]>
Co-authored-by: Joe C <[email protected]>
Co-authored-by: steviez <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Jacob Creech <[email protected]>
Co-authored-by: Nick Guo <[email protected]>
Co-authored-by: Ashwin Sekar <[email protected]>
Co-authored-by: Kevin Heavey <[email protected]>
Co-authored-by: Max Kaplan <[email protected]>
Co-authored-by: hugo-syn <[email protected]>
Co-authored-by: Andrew Fitzgerald <[email protected]>
Co-authored-by: norwnd <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Community contribution need:merge-assist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants