Integrate frostfsid into NNS #119

Merged
fyrchik merged 3 commits from achuprov/frostfs-contract:feat/integrate_frostfsid2nns into master 2024-11-20 15:43:58 +00:00
3 changed files with 107 additions and 3 deletions
Showing only changes of commit 645b4cb3c8 - Show all commits

39
nns/frostfsid.go Normal file
View file

@ -0,0 +1,39 @@
package nns
import (
"github.com/nspcc-dev/neo-go/pkg/interop"
"github.com/nspcc-dev/neo-go/pkg/interop/contract"
"github.com/nspcc-dev/neo-go/pkg/interop/native/management"
"github.com/nspcc-dev/neo-go/pkg/interop/native/std"
"github.com/nspcc-dev/neo-go/pkg/interop/storage"
)
const FrostfsIDNNSName = "frostfsid.frostfs"
const (
FrostfsIDNNSTLDPermissionKey = "nns-allow-register-tld"
FrostfsIDTLDRegistrationAllowed = "allow"
)

It is used only in tests, could you remove it?
We explicitly do allow check, everything else should be disallow.

It is used only in tests, could you remove it? We explicitly do allow check, _everything_ else should be disallow.
func checkFrostfsID(ctx storage.Context, addr interop.Hash160) bool {
if len(addr) == 0 {

We can store address in base-58 and use 29bb3ff1cf/pkg/core/native/std.go (L108)

We can store address in base-58 and use https://github.com/nspcc-dev/neo-go/blob/29bb3ff1cf4738ada24f075f5b398c1e70522778/pkg/core/native/std.go#L108

See #96

See https://git.frostfs.info/TrueCloudLab/frostfs-contract/issues/96

fixed

fixed
return false
}
frostfsIDAddress := getRecordsByType(ctx, []byte(tokenIDFromName(FrostfsIDNNSName)), FrostfsIDNNSName, TXT)
if len(frostfsIDAddress) < 2 {
return false
dkirillov marked this conversation as resolved Outdated

Should this be

if len(frostfsIDAddress) < 2 {

because below we use frostfsIDAddress[1]
?

Should this be ```golang if len(frostfsIDAddress) < 2 { ``` because below we use `frostfsIDAddress[1]` ?

Do we have a strict requirement to have the address in both Base56 and hex formats? The hex format does not affect functionality.

Do we have a strict requirement to have the address in both `Base56` and `hex` formats? The `hex` format does not affect functionality.

My comment was just about avoiding panic when result will contain just one element and we will try to access the second (frostfsIDAddress[1])

My comment was just about avoiding panic when result will contain just one element and we will try to access the second (`frostfsIDAddress[1]`)

You're right. Fixed.

You're right. Fixed.

I'm not sure. It seems you fixed wrong line

I'm not sure. It seems you fixed wrong line
}

Each signer has Scope, that is "bytecode area", where his signature is considered valid.
I may be OK with paying for this transaction (use None scope), but not ok with using my permissions to allow SOMEONE to do whatever they want.

Each signer has `Scope`, that is "bytecode area", where his signature is considered valid. I may be OK with paying for this transaction (use `None` scope), but not ok with using my permissions to allow SOMEONE to do whatever they want.

Another problem with this line of code is multiple contract.Call() invocations. This is too costly.

Another problem with this line of code is multiple `contract.Call()` invocations. This is too costly.

I think domain owner should be used here. He will own the domain, he should have the right to register.

I think domain owner should be used here. He will own the domain, he should have the right to register.

Each signer has Scope, that is "bytecode area", where his signature is considered valid.
I may be OK with paying for this transaction (use None scope), but not ok with using my permissions to allow SOMEONE to do whatever they want.

Just for my curiosity. You've said: "use None scope". Will be the contract method called then? I am looking at "getValueForSubjectKey" definition and it's expected that the method is invoked in the read context. So, why can't we use ReadOnly | AllowCall flags then?

> Each signer has `Scope`, that is "bytecode area", where his signature is considered valid. > I may be OK with paying for this transaction (use `None` scope), but not ok with using my permissions to allow SOMEONE to do whatever they want. Just for my curiosity. You've said: "use `None` scope". Will be the contract method called then? I am looking at `"getValueForSubjectKey"` definition and it's expected that the method is invoked in the read context. So, why can't we use `ReadOnly | AllowCall` flags then?

Signer scope (None, CalledByEntry etc) is different from the call flags (ReadOnly, AllowCall, etc).

Signer scope (`None`, `CalledByEntry` etc) is different from the call flags (`ReadOnly`, `AllowCall`, etc).

fixed

fixed
decodedBytes := std.Base58Decode([]byte(frostfsIDAddress[1]))
if len(decodedBytes) < 21 || management.GetContract(decodedBytes[1:21]) == nil {
return false
}
if res := contract.Call(decodedBytes[1:21], "getSubjectKV", contract.ReadOnly, addr, FrostfsIDNNSTLDPermissionKey).(string); res == FrostfsIDTLDRegistrationAllowed {
return true
}
return false
}

View file

@ -344,7 +344,7 @@ func register(ctx storage.Context, name string, owner interop.Hash160, email str
tldBytes := storage.Get(ctx, tldKey)
if countZone == 1 {
checkCommittee()
checkCommitteeAndFrostfsID(ctx, owner)
if tldBytes != nil {
panic("TLD already exists")
}
@ -924,6 +924,14 @@ func isValid(address interop.Hash160) bool {
return address != nil && len(address) == interop.Hash160Len
}
// checkCommitteeAndFrostfsID panics if the script container is not signed by the committee.
// or if the owner does not have permission in FrostfsID.
func checkCommitteeAndFrostfsID(ctx storage.Context, owner interop.Hash160) {
if !checkFrostfsID(ctx, owner) {
checkCommittee()
}
}
// checkCommittee panics if the script container is not signed by the committee.
func checkCommittee() {
committee := neo.GetCommittee()

View file

@ -12,10 +12,12 @@ import (
"github.com/nspcc-dev/neo-go/pkg/core/interop/storage"
"github.com/nspcc-dev/neo-go/pkg/core/state"
"github.com/nspcc-dev/neo-go/pkg/core/transaction"
"github.com/nspcc-dev/neo-go/pkg/encoding/address"
"github.com/nspcc-dev/neo-go/pkg/neotest"
"github.com/nspcc-dev/neo-go/pkg/rpcclient/gas"
"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/stretchr/testify/require"
)
@ -23,8 +25,7 @@ const nnsPath = "../nns"
const msPerYear = 365 * 24 * time.Hour / time.Millisecond
func newNNSInvoker(t *testing.T, addRoot bool) *neotest.ContractInvoker {
e := newExecutor(t)
func deployNNS(t *testing.T, e *neotest.Executor, addRoot bool) *neotest.ContractInvoker {
ctr := neotest.CompileFile(t, e.CommitteeHash, nnsPath, path.Join(nnsPath, "config.yml"))
e.DeployContract(t, ctr, nil)
@ -39,6 +40,28 @@ func newNNSInvoker(t *testing.T, addRoot bool) *neotest.ContractInvoker {
return c
}
func newNNSInvoker(t *testing.T, addRoot bool) *neotest.ContractInvoker {
e := newExecutor(t)
return deployNNS(t, e, addRoot)
}
func newNNSInvokerWithFrostfsID(t *testing.T, addRoot bool) (*neotest.ContractInvoker, *neotest.ContractInvoker) {
e := newExecutor(t)
c := deployNNS(t, e, addRoot)
frostfdID := deployFrostFSIDContract(t, e, e.CommitteeHash)
c.Invoke(t, true, "register",
nns.FrostfsIDNNSName, c.CommitteeHash,
"myemail@frostfs.info", defaultRefresh, defaultRetry, defaultExpire, defaultTTL)
c.Invoke(t, stackitem.Null{}, "addRecord",
nns.FrostfsIDNNSName, int64(nns.TXT), frostfdID.StringLE())
c.Invoke(t, stackitem.Null{}, "addRecord",
nns.FrostfsIDNNSName, int64(nns.TXT), address.Uint160ToString(frostfdID))

Please, let's not overcomplicate function signatures.
newNNSInvoker is perfectly valid.
We might have another newNNSInvokerWithFrostfsID which reuses newNNSInvoker and do not change unaffected tests. We need it only once (!).

Please, let's not overcomplicate function signatures. `newNNSInvoker` is perfectly valid. We might have another `newNNSInvokerWithFrostfsID` which reuses `newNNSInvoker` and do not change unaffected tests. We need it only once (!).

fixed

fixed
return e.NewInvoker(c.Hash), e.CommitteeInvoker(frostfdID)
}
func TestNNSGeneric(t *testing.T) {
c := newNNSInvoker(t, false)
@ -527,6 +550,40 @@ func TestNNSSetAdmin(t *testing.T) {
"testdomain.com", int64(nns.TXT), "will be added")
}
func TestNNS_Frostfsid(t *testing.T) {
nnsInv, f := newNNSInvokerWithFrostfsID(t, false)
acc, err := wallet.NewAccount()
require.NoError(t, err)
nnsUserInv := nnsInv.NewInvoker(nnsInv.Hash, newSigner(t, nnsInv, acc))
nnsUserInv.InvokeFail(t, "not witnessed by committee", "register",
"ddd", acc.ScriptHash(),
"myemail@frostfs.info", defaultRefresh, defaultRetry, defaultExpire, defaultTTL)
nnsUserInv.InvokeFail(t, "not witnessed by committee", "register",
"testdomain.kz", acc.ScriptHash(),
"myemail@frostfs.info", defaultRefresh, defaultRetry, defaultExpire, defaultTTL)
f.Invoke(t, stackitem.Null{}, createSubjectMethod, "", acc.PrivateKey().PublicKey().Bytes())
nnsUserInv.InvokeFail(t, "not witnessed by committee", "register",
"testdomain.kz", acc.ScriptHash(),
"myemail@frostfs.info", defaultRefresh, defaultRetry, defaultExpire, defaultTTL)
f.Invoke(t, stackitem.Null{}, setSubjectKVMethod, acc.ScriptHash(), nns.FrostfsIDNNSTLDPermissionKey, nns.FrostfsIDTLDRegistrationAllowed)
nnsUserInv.Invoke(t, true, "register",
"testdomain.kz", acc.ScriptHash(),
"myemail@frostfs.info", defaultRefresh, defaultRetry, defaultExpire, defaultTTL)
f.Invoke(t, stackitem.Null{}, deleteSubjectKVMethod, acc.ScriptHash(), nns.FrostfsIDNNSTLDPermissionKey)
nnsUserInv.InvokeFail(t, "not witnessed by committee", "register",
"testdomain.uz", acc.ScriptHash(),
"myemail@frostfs.info", defaultRefresh, defaultRetry, defaultExpire, defaultTTL)
}
func TestNNSIsAvailable(t *testing.T) {
c := newNNSInvoker(t, false)