adm: Allow use any wallets #1629
No reviewers
Labels
No labels
P0
P1
P2
P3
badger
frostfs-adm
frostfs-cli
frostfs-ir
frostfs-lens
frostfs-node
good first issue
triage
Infrastructure
blocked
bug
config
discussion
documentation
duplicate
enhancement
go
help wanted
internal
invalid
kludge
observability
perfomance
question
refactoring
wontfix
No milestone
No project
No assignees
5 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: TrueCloudLab/frostfs-node#1629
Loading…
Add table
Reference in a new issue
No description provided.
Delete branch "achuprov/frostfs-node:feat/frostfs_adm_allow_use_any_wallets"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Currently,
frostfs-adm
only allows the use ofalphabet-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:
e63080eb9f
tof96c1fe97d
@ -16,6 +16,12 @@ const (
EndpointFlagDesc = "N3 RPC node endpoint"
EndpointFlagShort = "r"
WalletPath = "wallet"
Is allowing to pass any wallet specific for NNS only? or are there any other use cases?
Right now, it’s just for
NNS
. But I don’t think it should apply only toNNS
. It could also be useful for other commands, likefrostfs-adm morph frostfsid create-subject
.@ -152,2 +155,4 @@
Run: frostfsidListGroupSubjects,
}
frostfsidAddKVCmd = &cobra.Command{
Just wondering, what is "kv subject"?
I think, adding this new command shouldn't be a part of the PR. Is this really necessary? What do you think?
The
NNS
contract allows registering aTLD
record only if the transaction has a consensus signature or if the subject has permission inFrostFSID
. Your comment made me refine the method to makekv-subject
more specific (frostfs-adm morph frostfsid allow-create-TL
).@ -19,0 +19,4 @@
WalletPath = "wallet"
WalletPathShorthand = "w"
WalletPathUsage = "Path to the wallet"
AdminWalletPath = "wallet-admin"
Who is admin?
New admin for NNS.
commonflags
package?wallet
?container
contract also hassetAdmin
.--wallet
can be the owner of the domain. We may need to set an admin in this case.@ -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)))
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).
fixed
@ -85,6 +106,14 @@ func openAlphabetWallets(v *viper.Viper, walletDir string) ([]*wallet.Wallet, er
return wallets, nil
}
func openWallet(walletPath string) (*wallet.Wallet, error) {
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.fixed
@ -65,0 +78,4 @@
}
func setAdmin(cmd *cobra.Command, _ []string) {
c, actor, accounts := nnsWriter(cmd)
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.
fixed
@ -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())
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.
fixed
@ -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}})
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.
fixed
f96c1fe97d
to6dfaabbcab
6dfaabbcab
to6a1ddfb5ed
@ -153,1 +156,4 @@
}
frostfsidAllowCreateTLDCmd = &cobra.Command{
Use: "allow-create-TLD",
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"
@ -236,6 +248,13 @@ func initFrostfsIDListGroupSubjectsCmd() {
frostfsidListGroupSubjectsCmd.Flags().Bool(includeNamesFlag, false, "Whether include subject name (require additional requests)")
}
func initFrostfsIDAllowCreateTLDCmd() {
This command doesn't require alphabet wallets, does it?
mv #1634
@ -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")
As for me, this interface looks strange
Do we already have something similar to that?
@ -406,0 +432,4 @@
if ok, _ := cmd.Flags().GetBool("disable"); ok {
permission = ""
}
ffsid.addCall(ffsid.roCli.SetSubjectKVCall(subjectAddress, nns.FrostfsIDNNSTLDPermissionKey, permission))
Could setting permissions be a part of
create-subject
?or a separate command?
Feel free to argue
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.@ -31,0 +51,4 @@
return w.GetAccount(w.GetChangeAddress()), nil
}
type AccountProviderPrm struct {
SimpleWallets
doesn't useLabel
Why this struct exists?
Having multiple
Path string
fields in different structures is (IMO, of course) more readable and simpler.fixed
@ -22,6 +23,27 @@ import (
"github.com/spf13/viper"
)
func GetWallet(_ *viper.Viper, walletPath string) (*wallet.Wallet, error) {
2 . I think that
GetWallet
should have a similar signature toGetAlphabetWallets
.1 . Additionally, regarding point 2,
viper
will be used to get the password from flags in the future.Why?
Then we will make this function public in the future.
IMO, two functions with similar behavior should have a similar interface (considering point 2).
Fixed
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)
What is the benefit of using
over
?
fixed
@ -41,3 +69,2 @@
wallets, err := GetAlphabetWallets(viper.GetViper(), walletDir)
commonCmd.ExitOnErr(cmd, "unable to get alphabet wallets: %w", err)
for _, p := range prm {
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?I tested it on Proxmox, it works (updated since your comment)
6a1ddfb5ed
to7281571120
7281571120
to5e8702d0ce
5e8702d0ce
tod796ef1932
d796ef1932
tocec28b9a11
adm: Allow use any walletsto WIP: adm: Allow use any walletscec28b9a11
to99d3081ea4
99d3081ea4
toe6bb53f6eb
e6bb53f6eb
to3b4d3aeac8
WIP: adm: Allow use any walletsto adm: Allow use any wallets@ -31,0 +59,4 @@
return []*wallet.Account{w.GetAccount(w.GetChangeAddress())}, nil
}
type AccountProviderPrm struct {
Now could you remove this one?
It is used once as an embedded struct, that complicates writing literal initialization.
deleted
@ -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) {
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 oneAlphabetWallets
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.fixed
3b4d3aeac8
to69eb69119a
69eb69119a
tod66d7aa926
d66d7aa926
to274c3f6b69
@ -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) {
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
or we have a specific reason to not do this?
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).@ -31,0 +46,4 @@
return accounts, nil
}
type SimpleWallets struct{ Path string }
Please, could we use name
RegularWallets
instead ofSimple
?Ok, renamed
274c3f6b69
to2fd30537ec
2fd30537ec
to4593555465
4593555465
toebf6a696e2
New commits pushed, approval review dismissed automatically according to repository settings
ebf6a696e2
to304bee938b