support more flexible manipulation of participation keys#96
support more flexible manipulation of participation keys#96zeldovich merged 12 commits intoalgorand:masterfrom
Conversation
Installs a partkey that was generated by algokey
| changeOnlineCmd.Flags().StringVarP(&accountAddress, "address", "a", "", "Account address to change (required)") | ||
| changeOnlineCmd.MarkFlagRequired("address") | ||
| changeOnlineCmd.Flags().StringVarP(&accountAddress, "address", "a", "", "Account address to change (required if no -partkeyfile)") | ||
| changeOnlineCmd.Flags().StringVarP(&partKeyFile, "partkeyfile", "", "", "Participation key file (required if no -account)") |
There was a problem hiding this comment.
Not important, but the StringVar() command exists (likewise for other data types) for commands that don't have the short-form parameter, so you don't have to specify "".
There was a problem hiding this comment.
Ah, good to know -- thanks!
| @@ -471,11 +477,36 @@ var changeOnlineCmd = &cobra.Command{ | |||
| Long: `Change online status for the specified account. Set online should be 1 to set online, 0 to set offline. The broadcast transaction will be valid for a limited number of rounds. goal will provide the TXID of the transaction if successful. Going online requires that the given account have a valid participation key.`, | |||
There was a problem hiding this comment.
While here, can we fix the help-text ('Going online requires that the given account have...' : have -> has)?
Also it might be confusing to people that this command does not 'install' the part file too - we should try to make sure there is no confusion.
There was a problem hiding this comment.
Indeed, clarified both.
| var installParticipationKeyCmd = &cobra.Command{ | ||
| Use: "installpartkey", | ||
| Short: "Install a participation key", | ||
| Long: `Install a participation key`, |
There was a problem hiding this comment.
We should indicate that this does not change online status.
| Run: func(cmd *cobra.Command, args []string) { | ||
| dataDir := ensureSingleDataDir() | ||
|
|
||
| client := ensureFullClient(dataDir) |
There was a problem hiding this comment.
We don't need a full client for this do we (just algod)?
There was a problem hiding this comment.
Yup, sloppy copy-pasting of libgoal boilerplate on my part.
| os.Exit(1) | ||
| } | ||
|
|
||
| partkey.Parent = parent |
There was a problem hiding this comment.
Rather than breaking the assignment and persistence, can we change PersistNewParent() to ReplaceParent(parent)?
There was a problem hiding this comment.
I was going for that originally, but for historical reasons, all current methods on account.Participation are by value rather than by pointer. So, ReplaceParent() would not have any apparent effect on the caller's partkey object (in particular, the subsequent printPartkey() would print the old parent), even though it could write the new parent to the partkey db.. Modifying the parent in-place seemed cleaner than having a mix of by-value and by-pointer methods on account.Participation, and changing all of them to by-pointer seemed unnecessarily involved.
| Use: "installpartkey", | ||
| Short: "Install a participation key", | ||
| Long: `Install a participation key`, | ||
| Long: `Install a participation key from a partkey file. Intended for use with participation key files generated by "algokey part generate". Does not change the online status of an account or register the participation key; use "goal account changeonlinestatus" for doing so.`, |
There was a problem hiding this comment.
We need to encourage proper partkey security. Should we delete the original partkey file if the installPartKey succeeds? If not, we should print out a message afterward to tell them they should delete it (or otherwise secure it).
There was a problem hiding this comment.
Yeah, you're probably right. I initially wasn't going to do this because deleting keys seemed risky, but in the case of install, sqlite guarantees durability for the key we just installed into the data directory, so it should be safe to delete because we know a copy is durably stored with algod now. Pushed code to do that.
| partkey.DeleteOldKeys(basics.Round(math.MaxUint64), config.Consensus[protocol.ConsensusCurrentVersion]) | ||
| os.Remove(inputfile) | ||
|
|
||
| return partkey, newdbpath, nil |
There was a problem hiding this comment.
Oops, you're right -- thanks for carefully looking it over!
There was a problem hiding this comment.
We're in production now - every review needs to be thorough... We should be more diligent about unit tests for all new code as well, but that's another discussion.
2 participants
algokey part generategenerates a new partkey, with the parent address being optional.algokey part infoprints info about a partkey file.algokey part reparentallows changing the parent address in a partkey file, which is important for some offline key management use cases.goal account installpartkeytakes an existing partkey file (from algokey, for example) and installs it into algod's directory.goal account changeonlinestatuscan now take a partkey file instead of an address, and in that case, it does not require that partkey to be already present in algod's directory.