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
Member

Currently, frostfs-adm only allows the use of alphabet-wallets. However, nns does not require domains to be alphabetic. Therefore, support for regular wallets has been added.
Account selection by label has not been implemented.
Example:

frostfs-adm morph nns set-admin -r=http://morph-chain.frostfs.devenv:30333 --name="zzzz" -w=services/s3_lifecycler/wallet.json --wallet-admin=/home/achuprov/Documents/work/frostfs-dev-env/services/s3_gate/wallet.json
Password for s3_lifecycler_wallet wallet > 
Password for s3_gate_wallet wallet > 
Set admin successfully
frostfs-adm morph nns add-record --data="ZZVV" --name="vvvv" -w=services/s3_lifecycler/wallet.json  --type=txt -r=http://morph-chain.frostfs.devenv:30333
Password for s3_lifecycler_wallet wallet > 
Waiting for transaction to persist...
Record added successfully
Currently, `frostfs-adm` only allows the use of `alphabet-wallets`. However, `nns` does not require domains to be alphabetic. Therefore, support for regular wallets has been added. Account selection by label has not been implemented. Example: ``` frostfs-adm morph nns set-admin -r=http://morph-chain.frostfs.devenv:30333 --name="zzzz" -w=services/s3_lifecycler/wallet.json --wallet-admin=/home/achuprov/Documents/work/frostfs-dev-env/services/s3_gate/wallet.json Password for s3_lifecycler_wallet wallet > Password for s3_gate_wallet wallet > Set admin successfully ``` ``` frostfs-adm morph nns add-record --data="ZZVV" --name="vvvv" -w=services/s3_lifecycler/wallet.json --type=txt -r=http://morph-chain.frostfs.devenv:30333 Password for s3_lifecycler_wallet wallet > Waiting for transaction to persist... Record added successfully ```
requested reviews from storage-core-committers, storage-core-developers 2025-01-31 17:19:31 +00:00
achuprov force-pushed feat/frostfs_adm_allow_use_any_wallets from e63080eb9f to f96c1fe97d 2025-01-31 17:20:32 +00:00 Compare
a-savchuk reviewed 2025-02-03 08:07:41 +00:00
@ -16,6 +16,12 @@ const (
EndpointFlagDesc = "N3 RPC node endpoint"
EndpointFlagShort = "r"
WalletPath = "wallet"
Member

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?
Author
Member

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`.
a-savchuk marked this conversation as resolved
@ -152,2 +155,4 @@
Run: frostfsidListGroupSubjects,
}
frostfsidAddKVCmd = &cobra.Command{
Member

Just wondering, what is "kv subject"?

Just wondering, what is "kv subject"?
Member

I think, adding this new command shouldn't be a part of the PR. Is this really necessary? What do you think?

I think, adding this new command shouldn't be a part of the PR. Is this really necessary? What do you think?
Author
Member

The NNS contract allows registering a TLD record only if the transaction has a consensus signature or if the subject has permission in FrostFSID. Your comment made me refine the method to make kv-subject more specific (frostfs-adm morph frostfsid allow-create-TL).

The `NNS` contract allows registering a `TLD` record only if the transaction has a consensus signature or if the subject has [permission](https://git.frostfs.info/TrueCloudLab/frostfs-contract/src/commit/201db45bd739fb729448ea8c806ae51ff01a5f2d/nns/frostfsid.go#L18) in `FrostFSID`. Your comment made me refine the method to make `kv-subject` more specific (`frostfs-adm morph frostfsid allow-create-TL`).
a-savchuk marked this conversation as resolved
fyrchik reviewed 2025-02-03 09:19:18 +00:00
@ -19,0 +19,4 @@
WalletPath = "wallet"
WalletPathShorthand = "w"
WalletPathUsage = "Path to the wallet"
AdminWalletPath = "wallet-admin"
Owner

Who is admin?

Who is admin?
Author
Member

New admin for NNS.

New admin for NNS.
Owner
  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`?
Author
Member
  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.
Owner
  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
Author
Member
  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.
fyrchik requested changes 2025-02-03 09:31:09 +00:00
@ -25,0 +29,4 @@
}
var password string
password, err = config.GetPassword(v, "", filepath.Base(filepath.Dir(walletPath))+"_"+strings.TrimSuffix(filepath.Base(walletPath), filepath.Ext(walletPath)))
Owner

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).
Author
Member

fixed

fixed
fyrchik marked this conversation as resolved
@ -85,6 +106,14 @@ func openAlphabetWallets(v *viper.Viper, walletDir string) ([]*wallet.Wallet, er
return wallets, nil
}
func openWallet(walletPath string) (*wallet.Wallet, error) {
Owner

Why is this function not completely useless?
It delegates work to other function and returns the same answer.
The only thing I see it being useful is if wallet.NewWalletFromFile returned non-nil wallet and non-nil error, and we would like to override that.

Why is this function not completely useless? It delegates work to other function and returns the same answer. The only thing I see it being useful is if `wallet.NewWalletFromFile` returned non-nil wallet and non-nil error, and we would like to override that.
Author
Member

fixed

fixed
fyrchik marked this conversation as resolved
@ -65,0 +78,4 @@
}
func setAdmin(cmd *cobra.Command, _ []string) {
c, actor, accounts := nnsWriter(cmd)
Owner

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.
Author
Member

fixed

fixed
fyrchik marked this conversation as resolved
@ -65,0 +81,4 @@
c, actor, accounts := nnsWriter(cmd)
name, _ := cmd.Flags().GetString(nnsNameFlag)
h, vub, err := c.SetAdmin(name, accounts[len(accounts)-1].ScriptHash())
Owner

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.
Author
Member

fixed

fixed
fyrchik marked this conversation as resolved
@ -19,1 +24,3 @@
ac, err := helper.NewLocalActor(cmd, c, constants.CommitteeAccountName)
var providers []helper.AccountProvider
if config.ResolveHomePath(viper.GetString(commonflags.AlphabetWalletsFlag)) != "" {
providers = append(providers, &helper.AlphabetWallets{AccountProviderPrm: helper.AccountProviderPrm{Path: config.ResolveHomePath(viper.GetString(commonflags.AlphabetWalletsFlag)), Label: constants.ConsensusAccountName}})
Owner

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.
Author
Member

fixed

fixed
fyrchik marked this conversation as resolved
achuprov force-pushed feat/frostfs_adm_allow_use_any_wallets from f96c1fe97d to 6dfaabbcab 2025-02-03 12:55:03 +00:00 Compare
achuprov force-pushed feat/frostfs_adm_allow_use_any_wallets from 6dfaabbcab to 6a1ddfb5ed 2025-02-03 13:13:37 +00:00 Compare
a-savchuk reviewed 2025-02-04 07:29:37 +00:00
@ -153,1 +156,4 @@
}
frostfsidAllowCreateTLDCmd = &cobra.Command{
Use: "allow-create-TLD",
Member

tld. I think, TLD in lower case would be OK. For example, you write it in lower case in a contract

FrostfsIDNNSTLDPermissionKey = "nns-allow-register-tld"

tld. I think, TLD in lower case would be OK. For example, you write it in lower case in a contract https://git.frostfs.info/TrueCloudLab/frostfs-contract/src/commit/201db45bd739fb729448ea8c806ae51ff01a5f2d/nns/frostfsid.go#L14
achuprov marked this conversation as resolved
@ -236,6 +248,13 @@ func initFrostfsIDListGroupSubjectsCmd() {
frostfsidListGroupSubjectsCmd.Flags().Bool(includeNamesFlag, false, "Whether include subject name (require additional requests)")
}
func initFrostfsIDAllowCreateTLDCmd() {
Member

This command doesn't require alphabet wallets, does it?

This command doesn't require alphabet wallets, does it?
Author
Member

mv #1634

mv https://git.frostfs.info/TrueCloudLab/frostfs-node/pulls/1634
achuprov marked this conversation as resolved
@ -239,0 +252,4 @@
Cmd.AddCommand(frostfsidAllowCreateTLDCmd)
frostfsidAllowCreateTLDCmd.Flags().StringP(commonflags.EndpointFlag, commonflags.EndpointFlagShort, "", commonflags.EndpointFlagDesc)
frostfsidAllowCreateTLDCmd.Flags().String(subjectAddressFlag, "", "Subject address")
frostfsidAllowCreateTLDCmd.Flags().Bool(disableFlag, false, "Deny subject from creating TLD domains")
Member

As for me, this interface looks strange

// allow creating TLD
frostfs-adm morph frostfsid allow-create-TLD

// deny creating TLD
frostfs-adm morph frostfsid allow-create-TLD --disable

Do we already have something similar to that?

As for me, this interface looks strange ``` // allow creating TLD frostfs-adm morph frostfsid allow-create-TLD // deny creating TLD frostfs-adm morph frostfsid allow-create-TLD --disable ``` Do we already have something similar to that?
achuprov marked this conversation as resolved
@ -406,0 +432,4 @@
if ok, _ := cmd.Flags().GetBool("disable"); ok {
permission = ""
}
ffsid.addCall(ffsid.roCli.SetSubjectKVCall(subjectAddress, nns.FrostfsIDNNSTLDPermissionKey, permission))
Member

Could setting permissions be a part of create-subject?

frostfs-adm morph frostfsid create-subject \
    --permissions=nns-allow-register-tld,<other permissions>...

or a separate command?

// allow
frostfs-adm morph frostfsid set-permissions nns-allow-register-tld=1,<another permission>=0

// deny
frostfs-adm morph frostfsid set-permissions nns-allow-register-tld=0,<another permission>=1

Feel free to argue

Could setting permissions be a part of `create-subject`? ``` frostfs-adm morph frostfsid create-subject \ --permissions=nns-allow-register-tld,<other permissions>... ``` or a separate command? ``` // allow frostfs-adm morph frostfsid set-permissions nns-allow-register-tld=1,<another permission>=0 // deny frostfs-adm morph frostfsid set-permissions nns-allow-register-tld=0,<another permission>=1 ``` Feel free to argue
Owner

For frostfsid I would prefer that we create abstractions either based on existing methods (setSubjectKV) or based on DID spec (https://w3c.github.io/did/).
permission is not taken from either.

For `frostfsid` I would prefer that we create abstractions either based on existing methods (`setSubjectKV`) or based on DID spec (https://w3c.github.io/did/). `permission` is not taken from either.
a-savchuk marked this conversation as resolved
fyrchik reviewed 2025-02-04 08:51:03 +00:00
@ -31,0 +51,4 @@
return w.GetAccount(w.GetChangeAddress()), nil
}
type AccountProviderPrm struct {
Owner

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.
Author
Member

fixed

fixed
achuprov marked this conversation as resolved
@ -22,6 +23,27 @@ import (
"github.com/spf13/viper"
)
func GetWallet(_ *viper.Viper, walletPath string) (*wallet.Wallet, error) {
Owner
  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?
Author
Member

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.
Owner

should have a similar signature

Why?

>should have a similar signature Why?
Owner

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.
Author
Member

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.
@ -25,0 +31,4 @@
password, err := input.ReadPassword("Enter password for wallet:")
if err != nil {
err = fmt.Errorf("can't fetch password: %w", err)
Owner

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) ``` ?
Author
Member

fixed

fixed
fyrchik marked this conversation as resolved
fyrchik reviewed 2025-02-04 08:57:42 +00:00
@ -41,3 +69,2 @@
wallets, err := GetAlphabetWallets(viper.GetViper(), walletDir)
commonCmd.ExitOnErr(cmd, "unable to get alphabet wallets: %w", err)
for _, p := range prm {
Owner

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?
Author
Member

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

I tested it on Proxmox, it works (updated since your comment)
achuprov force-pushed feat/frostfs_adm_allow_use_any_wallets from 6a1ddfb5ed to 7281571120 2025-02-04 13:08:32 +00:00 Compare
achuprov force-pushed feat/frostfs_adm_allow_use_any_wallets from 7281571120 to 5e8702d0ce 2025-02-04 14:38:52 +00:00 Compare
achuprov force-pushed feat/frostfs_adm_allow_use_any_wallets from 5e8702d0ce to d796ef1932 2025-02-04 14:39:20 +00:00 Compare
achuprov force-pushed feat/frostfs_adm_allow_use_any_wallets from d796ef1932 to cec28b9a11 2025-02-04 14:43:21 +00:00 Compare
fyrchik changed title from adm: Allow use any wallets to WIP: adm: Allow use any wallets 2025-02-05 11:23:12 +00:00
achuprov force-pushed feat/frostfs_adm_allow_use_any_wallets from cec28b9a11 to 99d3081ea4 2025-02-06 17:32:22 +00:00 Compare
achuprov force-pushed feat/frostfs_adm_allow_use_any_wallets from 99d3081ea4 to e6bb53f6eb 2025-02-06 17:36:01 +00:00 Compare
achuprov force-pushed feat/frostfs_adm_allow_use_any_wallets from e6bb53f6eb to 3b4d3aeac8 2025-02-06 17:41:27 +00:00 Compare
achuprov changed title from WIP: adm: Allow use any wallets to adm: Allow use any wallets 2025-02-06 18:11:20 +00:00
requested reviews from storage-core-committers, storage-core-developers 2025-02-06 18:11:20 +00:00
fyrchik reviewed 2025-02-07 10:58:20 +00:00
@ -31,0 +59,4 @@
return []*wallet.Account{w.GetAccount(w.GetChangeAddress())}, nil
}
type AccountProviderPrm struct {
Owner

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.
Author
Member

deleted

deleted
achuprov marked this conversation as resolved
fyrchik requested changes 2025-02-07 11:08:57 +00:00
@ -36,3 +72,2 @@
// 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, prm []AccountProvider) (*LocalActor, error) {
Owner

I think, this is a wrong abstraction.
See

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.
Author
Member

fixed

fixed
achuprov force-pushed feat/frostfs_adm_allow_use_any_wallets from 3b4d3aeac8 to 69eb69119a 2025-02-10 07:17:20 +00:00 Compare
achuprov force-pushed feat/frostfs_adm_allow_use_any_wallets from 69eb69119a to d66d7aa926 2025-02-10 12:14:50 +00:00 Compare
achuprov force-pushed feat/frostfs_adm_allow_use_any_wallets from d66d7aa926 to 274c3f6b69 2025-02-10 12:15:17 +00:00 Compare
aarifullin reviewed 2025-02-10 17:12:12 +00:00
@ -36,3 +65,2 @@
// 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, simple ...*SimpleWallets) (*LocalActor, error) {
Member

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?
Author
Member

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).
aarifullin marked this conversation as resolved
aarifullin reviewed 2025-02-10 17:13:10 +00:00
@ -31,0 +46,4 @@
return accounts, nil
}
type SimpleWallets struct{ Path string }
Member

Please, could we use name RegularWallets instead of Simple?

Please, could we use name `RegularWallets` instead of `Simple`?
Author
Member

Ok, renamed

Ok, renamed
aarifullin marked this conversation as resolved
achuprov force-pushed feat/frostfs_adm_allow_use_any_wallets from 274c3f6b69 to 2fd30537ec 2025-02-11 07:54:44 +00:00 Compare
achuprov force-pushed feat/frostfs_adm_allow_use_any_wallets from 2fd30537ec to 4593555465 2025-02-11 07:55:47 +00:00 Compare
aarifullin approved these changes 2025-02-11 08:10:06 +00:00
Dismissed
achuprov force-pushed feat/frostfs_adm_allow_use_any_wallets from 4593555465 to ebf6a696e2 2025-02-11 08:57:34 +00:00 Compare
achuprov dismissed aarifullin's review 2025-02-11 08:57:34 +00:00
Reason:

New commits pushed, approval review dismissed automatically according to repository settings

achuprov force-pushed feat/frostfs_adm_allow_use_any_wallets from ebf6a696e2 to 304bee938b 2025-02-11 09:01:11 +00:00 Compare
dstepanov-yadro approved these changes 2025-02-11 09:03:48 +00:00
aarifullin approved these changes 2025-02-11 11:01:12 +00:00
dstepanov-yadro merged commit 304bee938b into master 2025-02-11 11:04:31 +00:00
Sign in to join this conversation.
No milestone
No project
No assignees
5 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: TrueCloudLab/frostfs-node#1629
No description provided.