TrueCloudLab/frostfs-node
20
1
Fork 28
Code Issues 111 Pull requests 4 Projects Releases 2 Activity Actions

adm: Allow use any wallets #1629

Merged
dstepanov-yadro merged 2 commits from achuprov/frostfs-node:feat/frostfs_adm_allow_use_any_wallets into master 2025-02-11 11:04:31 +00:00
Conversation 0 Commits 2 Files changed 8 +182 -22
8 changed files with 182 additions and 22 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

7
cmd/frostfs-adm/internal/commonflags/flags.go
View file

@ -16,9 +16,16 @@ const (
EndpointFlagDesc = "N3 RPC node endpoint"
EndpointFlagShort = "r"
WalletPath = "wallet"
a-savchuk marked this conversation as resolved Outdated
a-savchuk commented 2025-02-03 08:07:37 +00:00
Outdated
Review
Copy link

Is allowing to pass any wallet specific for NNS only? or are there any other use cases?

Is allowing to pass any wallet specific for NNS only? or are there any other use cases?
achuprov commented 2025-02-03 08:58:14 +00:00
Outdated
Review
Copy link

Right now, it’s just for NNS. But I don’t think it should apply only to NNS. It could also be useful for other commands, like frostfs-adm morph frostfsid create-subject.

Right now, it’s just for `NNS`. But I don’t think it should apply only to `NNS`. It could also be useful for other commands, like `frostfs-adm morph frostfsid create-subject`.
WalletPathShorthand = "w"
WalletPathUsage = "Path to the wallet"
fyrchik commented 2025-02-03 09:19:18 +00:00
Outdated
Review
Copy link

Who is admin?

Who is admin?
achuprov commented 2025-02-03 14:22:28 +00:00
Outdated
Review
Copy link

New admin for NNS.

New admin for NNS.
fyrchik commented 2025-02-04 07:39:34 +00:00
Outdated
Review
Copy link
  1. This flag is used in 1 command, what does it do in commonflags package?
  2. Why there is a separate flag for this? Can't we have just wallet?
1. This flag is used in 1 command, what does it do in `commonflags` package? 2. Why there is a separate flag for this? Can't we have just `wallet`?
achuprov commented 2025-02-04 08:09:03 +00:00
Outdated
Review
Copy link
  1. The container contract also has setAdmin.
  2. --wallet can be the owner of the domain. We may need to set an admin in this case.
1. The `container` contract also has `setAdmin`. 2. `--wallet` can be the owner of the domain. We may need to set an admin in this case.
fyrchik commented 2025-02-04 09:48:35 +00:00
Outdated
Review
Copy link
  1. Yes, but it doesn't need 2 wallets. We need only admin and committee.
  2. Ok
1. Yes, but it doesn't need 2 wallets. We need only admin and committee. 2. Ok
achuprov commented 2025-02-10 15:32:16 +00:00
Outdated
Review
Copy link
  1. If we have only two wallets, the new admin and the committee. We won't be able to assign admin for the domain if its owner is a simple wallet because we won't have owner signature.
1. If we have only two wallets, the new admin and the committee. We won't be able to assign admin for the domain if its owner is a simple wallet [because](https://git.frostfs.info/TrueCloudLab/frostfs-contract/src/commit/201db45bd739fb729448ea8c806ae51ff01a5f2d/nns/nns_contract.go#L440) we won't have owner signature.
AlphabetWalletsFlag = "alphabet-wallets"
AlphabetWalletsFlagDesc = "Path to alphabet wallets dir"
AdminWalletPath = "wallet-admin"
AdminWalletUsage = "Path to the admin wallet"
LocalDumpFlag = "local-dump"
ProtoConfigPath = "protocol"
ContractsInitFlag = "contracts"

5
cmd/frostfs-adm/internal/modules/morph/ape/ape_util.go
View file

@ -3,6 +3,8 @@ package ape
import (
"errors"
"git.frostfs.info/TrueCloudLab/frostfs-node/cmd/frostfs-adm/internal/commonflags"
"git.frostfs.info/TrueCloudLab/frostfs-node/cmd/frostfs-adm/internal/modules/config"
"git.frostfs.info/TrueCloudLab/frostfs-node/cmd/frostfs-adm/internal/modules/morph/constants"
"git.frostfs.info/TrueCloudLab/frostfs-node/cmd/frostfs-adm/internal/modules/morph/helper"
commonCmd "git.frostfs.info/TrueCloudLab/frostfs-node/cmd/internal/common"
@ -76,7 +78,8 @@ func newPolicyContractInterface(cmd *cobra.Command) (*morph.ContractStorage, *he
c, err := helper.NewRemoteClient(viper.GetViper())
commonCmd.ExitOnErr(cmd, "unable to create NEO rpc client: %w", err)
ac, err := helper.NewLocalActor(cmd, c, constants.ConsensusAccountName)
walletDir := config.ResolveHomePath(viper.GetString(commonflags.AlphabetWalletsFlag))
ac, err := helper.NewLocalActor(c, &helper.AlphabetWallets{Path: walletDir, Label: constants.ConsensusAccountName})
commonCmd.ExitOnErr(cmd, "can't create actor: %w", err)
var ch util.Uint160

88
cmd/frostfs-adm/internal/modules/morph/helper/actor.go
View file

@ -3,9 +3,6 @@ package helper
import (
"fmt"
"git.frostfs.info/TrueCloudLab/frostfs-node/cmd/frostfs-adm/internal/commonflags"
"git.frostfs.info/TrueCloudLab/frostfs-node/cmd/frostfs-adm/internal/modules/config"
commonCmd "git.frostfs.info/TrueCloudLab/frostfs-node/cmd/internal/common"
"github.com/google/uuid"
"github.com/nspcc-dev/neo-go/pkg/core/state"
"github.com/nspcc-dev/neo-go/pkg/core/transaction"
@ -16,7 +13,6 @@ import (
"github.com/nspcc-dev/neo-go/pkg/util"
"github.com/nspcc-dev/neo-go/pkg/vm/stackitem"
"github.com/nspcc-dev/neo-go/pkg/wallet"
"github.com/spf13/cobra"
"github.com/spf13/viper"
)
@ -28,32 +24,86 @@ type LocalActor struct {
rpcInvoker invoker.RPCInvoke
}
type AlphabetWallets struct {
Label string
Path string
}
func (a *AlphabetWallets) GetAccount(v *viper.Viper) ([]*wallet.Account, error) {
w, err := GetAlphabetWallets(v, a.Path)
if err != nil {
return nil, err
}
var accounts []*wallet.Account
for _, wall := range w {
acc, err := GetWalletAccount(wall, a.Label)
if err != nil {
return nil, err
}
accounts = append(accounts, acc)
}
return accounts, nil
}
type RegularWallets struct{ Path string }
aarifullin marked this conversation as resolved Outdated
aarifullin commented 2025-02-10 17:13:10 +00:00
Outdated
Review
Copy link

Please, could we use name RegularWallets instead of Simple?

Please, could we use name `RegularWallets` instead of `Simple`?
👍 1
achuprov commented 2025-02-11 08:09:05 +00:00
Outdated
Review
Copy link

Ok, renamed

Ok, renamed
func (r *RegularWallets) GetAccount() ([]*wallet.Account, error) {
w, err := getRegularWallet(r.Path)
if err != nil {
return nil, err
achuprov marked this conversation as resolved Outdated
fyrchik commented 2025-02-04 07:56:42 +00:00
Outdated
Review
Copy link

SimpleWallets doesn't use Label
Why this struct exists?
Having multiple Path string fields in different structures is (IMO, of course) more readable and simpler.

`SimpleWallets` doesn't use `Label` Why this struct exists? Having multiple `Path string` fields in different structures is (IMO, of course) more readable and simpler.
achuprov commented 2025-02-06 17:48:07 +00:00
Outdated
Review
Copy link

fixed

fixed
}
return []*wallet.Account{w.GetAccount(w.GetChangeAddress())}, nil
}
// NewLocalActor create LocalActor with accounts form provided wallets.
// In case of empty wallets provided created actor with dummy account only for read operation.
//
achuprov marked this conversation as resolved Outdated
fyrchik commented 2025-02-07 10:58:08 +00:00
Outdated
Review
Copy link

Now could you remove this one?
It is used once as an embedded struct, that complicates writing literal initialization.

Now could you remove this one? It is used once as an embedded struct, that complicates writing literal initialization.
achuprov commented 2025-02-10 15:13:42 +00:00
Outdated
Review
Copy link

deleted

deleted
// If wallets are provided, the contract client will use accounts with accName name from these wallets.
// To determine which account name should be used in a contract client, refer to how the contract
// verifies the transaction signature.
func NewLocalActor(cmd *cobra.Command, c actor.RPCActor, accName string) (*LocalActor, error) {
walletDir := config.ResolveHomePath(viper.GetString(commonflags.AlphabetWalletsFlag))
func NewLocalActor(c actor.RPCActor, alphabet *AlphabetWallets, regularWallets ...*RegularWallets) (*LocalActor, error) {
aarifullin marked this conversation as resolved Outdated
aarifullin commented 2025-02-10 17:12:12 +00:00
Outdated
Review
Copy link

May we have a situation when the actor should sign not only by alphabet wallets but also simple wallets (alphabet + simple)?
Do I correctly understand this is a way to select wallets in one of manner? If it's so, why couldn't we introduce

func NewLocalActorWithAlphabetWallets(c actor.RPCActor, alphabet *AlphabetWallets) (*LocalActor, error)

func NewLocalActorWithSimpleWallets(c actor.RPCActor, alphabet *SimpleWallets) (*LocalActor, error)

or we have a specific reason to not do this?

May we have a situation when the actor should sign not only by alphabet wallets but also simple wallets (alphabet + simple)? Do I correctly understand this is a way to select wallets in _one of_ manner? If it's so, why couldn't we introduce ```go func NewLocalActorWithAlphabetWallets(c actor.RPCActor, alphabet *AlphabetWallets) (*LocalActor, error) func NewLocalActorWithSimpleWallets(c actor.RPCActor, alphabet *SimpleWallets) (*LocalActor, error) ``` or we have a specific reason to not do this?
achuprov commented 2025-02-11 08:07:16 +00:00
Outdated
Review
Copy link

Yes, we may encounter situations where we need a combined signature from both alphabet wallets and regular wallets. For example, NNS register may require the committee signature along with the owner's signature (where the owner can be a regular wallet).

Yes, we may encounter situations where we need a combined signature from both alphabet wallets and regular wallets. For example, `NNS` [register](https://git.frostfs.info/TrueCloudLab/frostfs-contract/src/commit/201db45bd739fb729448ea8c806ae51ff01a5f2d/nns/nns_contract.go#L339) may require the committee signature along with the owner's signature (where the owner can be a regular wallet).
var act *actor.Actor
var accounts []*wallet.Account
var signers []actor.SignerAccount
fyrchik commented 2025-02-04 08:57:38 +00:00
Outdated
Review
Copy link

Previously we had all committee wallets in accounts array, it seems now we have only 1.
Could you check ape add-rule-chain with a 4-node consensus?

Previously we had all committee wallets in `accounts` array, it seems now we have only 1. Could you check `ape add-rule-chain` with a 4-node consensus?
achuprov commented 2025-02-06 17:47:49 +00:00
Outdated
Review
Copy link

I tested it on Proxmox, it works (updated since your comment)

I tested it on Proxmox, it works (updated since your comment)
wallets, err := GetAlphabetWallets(viper.GetViper(), walletDir)
commonCmd.ExitOnErr(cmd, "unable to get alphabet wallets: %w", err)
if alphabet != nil {
account, err := alphabet.GetAccount(viper.GetViper())
if err != nil {
fyrchik commented 2025-02-07 11:08:50 +00:00
Outdated
Review
Copy link

I think, this is a wrong abstraction.
See

Line 97 in 69c35b1
if len(a.accounts[0].Contract.Parameters) > 1 {

We use accounts to resign transaction and use it in a pretty specific way.
Thus, we do not accept any AccountProvider here, we accept at most one AlphabetWallets provider, which must always be first. It is not reflected in the function interface.
I would rather have sth like NewLocalActor(actor.RPCActor, *AlphabetWallets, ...SimpleWallets)
This way no artifical interface is needed and loop over []SimpleAccounts is uniform.

I think, this is a wrong abstraction. See https://git.frostfs.info/TrueCloudLab/frostfs-node/src/commit/69c35b1d61fad2d08edf008f709f19ad45fdf250/cmd/frostfs-adm/internal/modules/morph/helper/actor.go#L97 We use `accounts` to resign transaction and use it in a pretty specific way. Thus, we do not accept any `AccountProvider` here, we accept _at most one_ `AlphabetWallets` provider, which must always be first. It is not reflected in the function interface. I would rather have sth like `NewLocalActor(actor.RPCActor, *AlphabetWallets, ...SimpleWallets)` This way no artifical interface is needed and loop over `[]SimpleAccounts` is uniform.
achuprov commented 2025-02-10 12:39:11 +00:00
Outdated
Review
Copy link

fixed

fixed
return nil, err
}
for _, w := range wallets {
acc, err := GetWalletAccount(w, accName)
commonCmd.ExitOnErr(cmd, fmt.Sprintf("can't find %s account: %%w", accName), err)
accounts = append(accounts, acc)
accounts = append(accounts, account...)
signers = append(signers, actor.SignerAccount{
Signer: transaction.Signer{
Account: account[0].Contract.ScriptHash(),
Scopes: transaction.Global,
},
Account: account[0],
})
}
act, err = actor.New(c, []actor.SignerAccount{{
Signer: transaction.Signer{
Account: accounts[0].Contract.ScriptHash(),
Scopes: transaction.Global,
},
Account: accounts[0],
}})
for _, w := range regularWallets {
if w == nil {
continue
}
account, err := w.GetAccount()
if err != nil {
return nil, err
}
accounts = append(accounts, account...)
signers = append(signers, actor.SignerAccount{
Signer: transaction.Signer{
Account: account[0].Contract.ScriptHash(),
Scopes: transaction.Global,
},
Account: account[0],
})
}
act, err := actor.New(c, signers)
if err != nil {
return nil, err
}

24
cmd/frostfs-adm/internal/modules/morph/helper/util.go
View file

@ -14,6 +14,7 @@ import (
"git.frostfs.info/TrueCloudLab/frostfs-node/cmd/frostfs-adm/internal/modules/config"
"git.frostfs.info/TrueCloudLab/frostfs-node/cmd/frostfs-adm/internal/modules/morph/constants"
"git.frostfs.info/TrueCloudLab/frostfs-node/pkg/innerring"
"github.com/nspcc-dev/neo-go/cli/input"
"github.com/nspcc-dev/neo-go/pkg/core/state"
"github.com/nspcc-dev/neo-go/pkg/crypto/keys"
"github.com/nspcc-dev/neo-go/pkg/encoding/fixedn"
@ -22,6 +23,27 @@ import (
"github.com/spf13/viper"
)
func getRegularWallet(walletPath string) (*wallet.Wallet, error) {
fyrchik commented 2025-02-04 07:52:24 +00:00
Outdated
Review
Copy link
  1. What is the first parameter here for?
  2. Why is this function public?
1. What is the first parameter here for? 2. Why is this function public?
achuprov commented 2025-02-04 14:21:29 +00:00
Outdated
Review
Copy link

2 . I think that GetWallet should have a similar signature to GetAlphabetWallets.
1 . Additionally, regarding point 2, viper will be used to get the password from flags in the future.

2 . I think that `GetWallet` should have a similar signature to `GetAlphabetWallets`. 1 . Additionally, regarding point 2, `viper` will be used to get the password from flags in the future.
fyrchik commented 2025-02-05 11:21:38 +00:00
Outdated
Review
Copy link

should have a similar signature

Why?

>should have a similar signature Why?
fyrchik commented 2025-02-05 11:22:03 +00:00
Outdated
Review
Copy link

will be used to get the password from flags in the future

Then we will make this function public in the future.

>will be used to get the password from flags _in the future_ Then we will make this function public in the future.
achuprov commented 2025-02-06 18:11:14 +00:00
Outdated
Review
Copy link

Should this function have a similar signature?
Why?

IMO, two functions with similar behavior should have a similar interface (considering point 2).
Fixed

Then we will make this function public in the future.

Ok, I have made this function private.

> Should this function have a similar signature? > Why? IMO, two functions with similar behavior should have a similar interface (considering point 2). Fixed > Then we will make this function public in the future. Ok, I have made this function private.
w, err := wallet.NewWalletFromFile(walletPath)
if err != nil {
return nil, err
}
password, err := input.ReadPassword("Enter password for wallet:")
fyrchik marked this conversation as resolved Outdated
fyrchik commented 2025-02-03 09:23:03 +00:00
Outdated
Review
Copy link

credentials is a section exactly to store credentials.
Please, let's not add complexity here.
For custom wallets, I would avoid using config completely (at least for now).

`credentials` is a section exactly to store credentials. Please, let's not add complexity here. For custom wallets, I would avoid using config completely (at least for now).
achuprov commented 2025-02-03 14:21:42 +00:00
Outdated
Review
Copy link

fixed

fixed
if err != nil {
return nil, fmt.Errorf("can't fetch password: %w", err)
fyrchik marked this conversation as resolved Outdated
fyrchik commented 2025-02-04 07:54:38 +00:00
Outdated
Review
Copy link

What is the benefit of using

err = fmt.Errorf("can't fetch password: %w", err)
return nil, err

over

return nil, fmt.Errorf("can't fetch password: %w", err)

?

What is the benefit of using ``` err = fmt.Errorf("can't fetch password: %w", err) return nil, err ``` over ``` return nil, fmt.Errorf("can't fetch password: %w", err) ``` ?
achuprov commented 2025-02-04 14:49:34 +00:00
Outdated
Review
Copy link

fixed

fixed
}
for i := range w.Accounts {
if err = w.Accounts[i].Decrypt(password, keys.NEP2ScryptParams()); err != nil {
err = fmt.Errorf("can't unlock wallet: %w", err)
break
}
}
return w, err
}
func GetAlphabetWallets(v *viper.Viper, walletDir string) ([]*wallet.Wallet, error) {
wallets, err := openAlphabetWallets(v, walletDir)
if err != nil {
@ -51,7 +73,7 @@ func openAlphabetWallets(v *viper.Viper, walletDir string) ([]*wallet.Wallet, er
if errors.Is(err, os.ErrNotExist) {
err = nil
} else {
err = fmt.Errorf("can't open wallet: %w", err)
err = fmt.Errorf("can't open alphabet wallet: %w", err)
}
break
}

29
cmd/frostfs-adm/internal/modules/morph/nns/domains.go
View file

@ -6,7 +6,9 @@ import (
"git.frostfs.info/TrueCloudLab/frostfs-node/cmd/frostfs-adm/internal/commonflags"
"git.frostfs.info/TrueCloudLab/frostfs-node/cmd/frostfs-adm/internal/modules/morph/constants"
commonCmd "git.frostfs.info/TrueCloudLab/frostfs-node/cmd/internal/common"
"github.com/nspcc-dev/neo-go/pkg/wallet"
"github.com/spf13/cobra"
"github.com/spf13/viper"
)
func initRegisterCmd() {
@ -19,6 +21,7 @@ func initRegisterCmd() {
registerCmd.Flags().Int64(nnsRetryFlag, constants.NNSRetryDefVal, "SOA record RETRY parameter")
registerCmd.Flags().Int64(nnsExpireFlag, int64(constants.DefaultExpirationTime), "SOA record EXPIRE parameter")
registerCmd.Flags().Int64(nnsTTLFlag, constants.NNSTtlDefVal, "SOA record TTL parameter")
registerCmd.Flags().StringP(commonflags.WalletPath, commonflags.WalletPathShorthand, "", commonflags.WalletPathUsage)
_ = cobra.MarkFlagRequired(registerCmd.Flags(), nnsNameFlag)
}
@ -48,6 +51,7 @@ func initDeleteCmd() {
deleteCmd.Flags().StringP(commonflags.EndpointFlag, commonflags.EndpointFlagShort, "", commonflags.EndpointFlagDesc)
deleteCmd.Flags().String(commonflags.AlphabetWalletsFlag, "", commonflags.AlphabetWalletsFlagDesc)
deleteCmd.Flags().String(nnsNameFlag, "", nnsNameFlagDesc)
deleteCmd.Flags().StringP(commonflags.WalletPath, commonflags.WalletPathShorthand, "", commonflags.WalletPathUsage)
_ = cobra.MarkFlagRequired(deleteCmd.Flags(), nnsNameFlag)
}
@ -62,3 +66,28 @@ func deleteDomain(cmd *cobra.Command, _ []string) {
commonCmd.ExitOnErr(cmd, "delete domain error: %w", err)
cmd.Println("Domain deleted successfully")
}
func initSetAdminCmd() {
Cmd.AddCommand(setAdminCmd)
setAdminCmd.Flags().StringP(commonflags.EndpointFlag, commonflags.EndpointFlagShort, "", commonflags.EndpointFlagDesc)
setAdminCmd.Flags().String(commonflags.AlphabetWalletsFlag, "", commonflags.AlphabetWalletsFlagDesc)
setAdminCmd.Flags().String(nnsNameFlag, "", nnsNameFlagDesc)
setAdminCmd.Flags().StringP(commonflags.WalletPath, commonflags.WalletPathShorthand, "", commonflags.WalletPathUsage)
setAdminCmd.Flags().String(commonflags.AdminWalletPath, "", commonflags.AdminWalletUsage)
_ = setAdminCmd.MarkFlagRequired(commonflags.AdminWalletPath)
_ = cobra.MarkFlagRequired(setAdminCmd.Flags(), nnsNameFlag)
}
fyrchik marked this conversation as resolved Outdated
fyrchik commented 2025-02-03 09:29:29 +00:00
Outdated
Review
Copy link

You ignore the third return flag in all but one commands.
It is worth to move the complexity in that command instead and not ignore return value in every other place.

You ignore the third return flag in all but one commands. It is worth to move the complexity in that command instead and not ignore return value in every other place.
achuprov commented 2025-02-03 14:21:29 +00:00
Outdated
Review
Copy link

fixed

fixed
func setAdmin(cmd *cobra.Command, _ []string) {
c, actor := nnsWriter(cmd)
fyrchik marked this conversation as resolved Outdated
fyrchik commented 2025-02-03 09:30:51 +00:00
Outdated
Review
Copy link

I don't like using the last returned value to set admin, because accounts slice depends on the environment.
It should be obvious from the setAdmin function alone, what my code does.
The admin should be explicit.

I don't like using the last returned value to set admin, because `accounts` slice depends on the environment. It should be obvious from the `setAdmin` function alone, what my code does. The _admin_ should be explicit.
achuprov commented 2025-02-03 14:21:19 +00:00
Outdated
Review
Copy link

fixed

fixed
name, _ := cmd.Flags().GetString(nnsNameFlag)
w, err := wallet.NewWalletFromFile(viper.GetString(commonflags.AdminWalletPath))
commonCmd.ExitOnErr(cmd, "can't get admin wallet: %w", err)
h, vub, err := c.SetAdmin(name, w.GetAccount(w.GetChangeAddress()).ScriptHash())
_, err = actor.Wait(h, vub, err)
commonCmd.ExitOnErr(cmd, "Set admin error: %w", err)
cmd.Println("Set admin successfully")
}

31
cmd/frostfs-adm/internal/modules/morph/nns/helper.go
View file

@ -1,7 +1,11 @@
package nns
import (
"errors"
client "git.frostfs.info/TrueCloudLab/frostfs-contract/rpcclient/nns"
"git.frostfs.info/TrueCloudLab/frostfs-node/cmd/frostfs-adm/internal/commonflags"
"git.frostfs.info/TrueCloudLab/frostfs-node/cmd/frostfs-adm/internal/modules/config"
"git.frostfs.info/TrueCloudLab/frostfs-node/cmd/frostfs-adm/internal/modules/morph/constants"
"git.frostfs.info/TrueCloudLab/frostfs-node/cmd/frostfs-adm/internal/modules/morph/helper"
commonCmd "git.frostfs.info/TrueCloudLab/frostfs-node/cmd/internal/common"
@ -16,7 +20,32 @@ func nnsWriter(cmd *cobra.Command) (*client.Contract, *helper.LocalActor) {
c, err := helper.NewRemoteClient(v)
commonCmd.ExitOnErr(cmd, "unable to create NEO rpc client: %w", err)
ac, err := helper.NewLocalActor(cmd, c, constants.CommitteeAccountName)
alphabetWalletPath := config.ResolveHomePath(v.GetString(commonflags.AlphabetWalletsFlag))
walletPath := config.ResolveHomePath(v.GetString(commonflags.WalletPath))
adminWalletPath := config.ResolveHomePath(v.GetString(commonflags.AdminWalletPath))
fyrchik marked this conversation as resolved Outdated
fyrchik commented 2025-02-03 09:26:55 +00:00
Outdated
Review
Copy link

You add a provider if the flag is defined.
This is wrong IMO, --wallet should be used as an override.
So that I have default config for most of my cases (with alphabet wallets and rpc-endpoint), but can override wallet for one command.

You add a provider if the flag is defined. This is wrong IMO, `--wallet` should be used as an override. So that I have default config for most of my cases (with alphabet wallets and rpc-endpoint), but can override wallet for one command.
achuprov commented 2025-02-03 14:21:25 +00:00
Outdated
Review
Copy link

fixed

fixed
var (
alphabet *helper.AlphabetWallets
regularWallets []*helper.RegularWallets
)
if alphabetWalletPath != "" {
alphabet = &helper.AlphabetWallets{Path: alphabetWalletPath, Label: constants.ConsensusAccountName}
}
if walletPath != "" {
regularWallets = append(regularWallets, &helper.RegularWallets{Path: walletPath})
}
if adminWalletPath != "" {
regularWallets = append(regularWallets, &helper.RegularWallets{Path: adminWalletPath})
}
if alphabet == nil && regularWallets == nil {
commonCmd.ExitOnErr(cmd, "", errors.New("no wallets provided"))
}
ac, err := helper.NewLocalActor(c, alphabet, regularWallets...)
commonCmd.ExitOnErr(cmd, "can't create actor: %w", err)
r := management.NewReader(ac.Invoker)

3
cmd/frostfs-adm/internal/modules/morph/nns/record.go
View file

@ -19,6 +19,7 @@ func initAddRecordCmd() {
addRecordCmd.Flags().String(nnsNameFlag, "", nnsNameFlagDesc)
addRecordCmd.Flags().String(nnsRecordTypeFlag, "", nnsRecordTypeFlagDesc)
addRecordCmd.Flags().String(nnsRecordDataFlag, "", nnsRecordDataFlagDesc)
addRecordCmd.Flags().StringP(commonflags.WalletPath, commonflags.WalletPathShorthand, "", commonflags.WalletPathUsage)
_ = cobra.MarkFlagRequired(addRecordCmd.Flags(), nnsNameFlag)
_ = cobra.MarkFlagRequired(addRecordCmd.Flags(), nnsRecordTypeFlag)
@ -40,6 +41,7 @@ func initDelRecordsCmd() {
delRecordsCmd.Flags().String(commonflags.AlphabetWalletsFlag, "", commonflags.AlphabetWalletsFlagDesc)
delRecordsCmd.Flags().String(nnsNameFlag, "", nnsNameFlagDesc)
delRecordsCmd.Flags().String(nnsRecordTypeFlag, "", nnsRecordTypeFlagDesc)
delRecordsCmd.Flags().StringP(commonflags.WalletPath, commonflags.WalletPathShorthand, "", commonflags.WalletPathUsage)
_ = cobra.MarkFlagRequired(delRecordsCmd.Flags(), nnsNameFlag)
_ = cobra.MarkFlagRequired(delRecordsCmd.Flags(), nnsRecordTypeFlag)
@ -52,6 +54,7 @@ func initDelRecordCmd() {
delRecordCmd.Flags().String(nnsNameFlag, "", nnsNameFlagDesc)
delRecordCmd.Flags().String(nnsRecordTypeFlag, "", nnsRecordTypeFlagDesc)
delRecordCmd.Flags().String(nnsRecordDataFlag, "", nnsRecordDataFlagDesc)
delRecordCmd.Flags().StringP(commonflags.WalletPath, commonflags.WalletPathShorthand, "", commonflags.WalletPathUsage)
_ = cobra.MarkFlagRequired(delRecordCmd.Flags(), nnsNameFlag)
_ = cobra.MarkFlagRequired(delRecordCmd.Flags(), nnsRecordTypeFlag)

17
cmd/frostfs-adm/internal/modules/morph/nns/root.go
View file

@ -39,6 +39,7 @@ var (
PreRun: func(cmd *cobra.Command, _ []string) {
_ = viper.BindPFlag(commonflags.EndpointFlag, cmd.Flags().Lookup(commonflags.EndpointFlag))
_ = viper.BindPFlag(commonflags.AlphabetWalletsFlag, cmd.Flags().Lookup(commonflags.AlphabetWalletsFlag))
_ = viper.BindPFlag(commonflags.WalletPath, cmd.Flags().Lookup(commonflags.WalletPath))
},
Run: registerDomain,
}
@ -48,6 +49,7 @@ var (
PreRun: func(cmd *cobra.Command, _ []string) {
_ = viper.BindPFlag(commonflags.EndpointFlag, cmd.Flags().Lookup(commonflags.EndpointFlag))
_ = viper.BindPFlag(commonflags.AlphabetWalletsFlag, cmd.Flags().Lookup(commonflags.AlphabetWalletsFlag))
_ = viper.BindPFlag(commonflags.WalletPath, cmd.Flags().Lookup(commonflags.WalletPath))
},
Run: deleteDomain,
}
@ -75,6 +77,7 @@ var (
PreRun: func(cmd *cobra.Command, _ []string) {
_ = viper.BindPFlag(commonflags.EndpointFlag, cmd.Flags().Lookup(commonflags.EndpointFlag))
_ = viper.BindPFlag(commonflags.AlphabetWalletsFlag, cmd.Flags().Lookup(commonflags.AlphabetWalletsFlag))
_ = viper.BindPFlag(commonflags.WalletPath, cmd.Flags().Lookup(commonflags.WalletPath))
},
Run: addRecord,
}
@ -92,6 +95,7 @@ var (
PreRun: func(cmd *cobra.Command, _ []string) {
_ = viper.BindPFlag(commonflags.EndpointFlag, cmd.Flags().Lookup(commonflags.EndpointFlag))
_ = viper.BindPFlag(commonflags.AlphabetWalletsFlag, cmd.Flags().Lookup(commonflags.AlphabetWalletsFlag))
_ = viper.BindPFlag(commonflags.WalletPath, cmd.Flags().Lookup(commonflags.WalletPath))
},
Run: delRecords,
}
@ -101,9 +105,21 @@ var (
PreRun: func(cmd *cobra.Command, _ []string) {
_ = viper.BindPFlag(commonflags.EndpointFlag, cmd.Flags().Lookup(commonflags.EndpointFlag))
_ = viper.BindPFlag(commonflags.AlphabetWalletsFlag, cmd.Flags().Lookup(commonflags.AlphabetWalletsFlag))
_ = viper.BindPFlag(commonflags.WalletPath, cmd.Flags().Lookup(commonflags.WalletPath))
},
Run: delRecord,
}
setAdminCmd = &cobra.Command{
Use: "set-admin",
Short: "Sets admin for domain",
PreRun: func(cmd *cobra.Command, _ []string) {
_ = viper.BindPFlag(commonflags.EndpointFlag, cmd.Flags().Lookup(commonflags.EndpointFlag))
_ = viper.BindPFlag(commonflags.AlphabetWalletsFlag, cmd.Flags().Lookup(commonflags.AlphabetWalletsFlag))
_ = viper.BindPFlag(commonflags.WalletPath, cmd.Flags().Lookup(commonflags.WalletPath))
_ = viper.BindPFlag(commonflags.AdminWalletPath, cmd.Flags().Lookup(commonflags.AdminWalletPath))
},
Run: setAdmin,
}
)
func init() {
@ -116,4 +132,5 @@ func init() {
initGetRecordsCmd()
initDelRecordsCmd()
initDelRecordCmd()
initSetAdminCmd()
}