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(cmd): Add support for registry operations #47

Merged
merged 1 commit into from
Mar 30, 2023

Conversation

kostko
Copy link
Member

@kostko kostko commented Mar 28, 2023

No description provided.

@kostko kostko force-pushed the kostko/feature/registry-unfreezenode branch 2 times, most recently from 51f62dd to 7fc47e9 Compare March 28, 2023 17:11
@kostko kostko requested a review from matevz March 28, 2023 17:31
Copy link
Member

@matevz matevz left a comment

Choose a reason for hiding this comment

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

Few questions:

  1. Where is nodeRegister operation?
  2. Should we move oasis inspect registry to something like oasis registry show instead? Alsooasis registry dump would come in handy.

cmd/registry.go Outdated Show resolved Hide resolved
@kostko
Copy link
Member Author

kostko commented Mar 29, 2023

Node registration happens automatically by oasis-node so manual registration should never be needed.

Good point about the registry inspect functions. By dump you probably mean something like the current inspect registry nodes?

@kostko kostko force-pushed the kostko/feature/registry-unfreezenode branch from 7fc47e9 to baf2c9a Compare March 30, 2023 10:09
@kostko
Copy link
Member Author

kostko commented Mar 30, 2023

Moved the inspect registry part as well.

@kostko kostko requested a review from matevz March 30, 2023 10:29
@kostko kostko enabled auto-merge March 30, 2023 10:30
Copy link
Member

@matevz matevz left a comment

Choose a reason for hiding this comment

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

Good job!

cobra.CheckErr("account does not support signing consensus transactions")
}
if !signer.Public().Equal(descriptor.ID) {
cobra.CheckErr(fmt.Errorf("entity ID (%s) does not correspond to selected account (%s)",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
cobra.CheckErr(fmt.Errorf("entity ID (%s) does not correspond to selected account (%s)",
cobra.CheckErr(fmt.Errorf("entity ID '%s' does not correspond to selected account '%s' (%s)",

}
if !signer.Public().Equal(descriptor.ID) {
cobra.CheckErr(fmt.Errorf("entity ID (%s) does not correspond to selected account (%s)",
descriptor.ID, signer.Public()))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
descriptor.ID, signer.Public()))
descriptor.ID, npa.AccountName, signer.Public()))

acc := common.LoadAccount(cfg, npa.AccountName)
signer := acc.ConsensusSigner()
if signer == nil {
cobra.CheckErr("account does not support signing consensus transactions")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
cobra.CheckErr("account does not support signing consensus transactions")
cobra.CheckErr(fmt.Errorf("account '%s' does not support signing consensus transactions", npa.AccountName))

tx := registry.NewDeregisterEntityTx(0, nil)

acc := common.LoadAccount(cfg, npa.AccountName)
sigTx, err := common.SignConsensusTransaction(ctx, npa, acc, conn, tx)
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice if PrettyPrinter omitted the

Body:
  {}

chunk if Body is empty. But this needs to be fixed in SDK.

registryRuntimeRegisterCmd.Flags().AddFlagSet(common.SelectorFlags)
registryRuntimeRegisterCmd.Flags().AddFlagSet(common.TransactionFlags)

registryShowCmd.Flags().AddFlagSet(common.SelectorFlags)
Copy link
Member

Choose a reason for hiding this comment

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

oasis registry show only requires --network flag afaik.

@kostko kostko merged commit 7dca158 into master Mar 30, 2023
@kostko kostko deleted the kostko/feature/registry-unfreezenode branch March 30, 2023 14:56
@matevz
Copy link
Member

matevz commented Mar 30, 2023

Ah, crap, automerge was enabled. Can you make a short followup?

@kostko
Copy link
Member Author

kostko commented Mar 30, 2023

Yeah will do.

@kostko
Copy link
Member Author

kostko commented Mar 30, 2023

@matevz should be addressed in #49.

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.

None yet

2 participants