TrueCloudLab/frostfs-contract
21
0
Fork 19
Code Issues 12 Pull requests 2 Projects Releases 2 Activity Actions

Integrate frostfsid into NNS #119

Merged
fyrchik merged 3 commits from achuprov/frostfs-contract:feat/integrate_frostfsid2nns into master 2024-11-05 14:58:57 +00:00
Conversation 0 Commits 3 Files changed 8 +176 -3
achuprov commented 2024-10-21 11:30:00 +00:00
Member
Copy link
No description provided.
achuprov added 2 commits 2024-10-21 11:30:00 +00:00 
Signed-off-by: Alexander Chuprov <a.chuprov@yadro.com>
[#118] nns: Integrate 'frostfsid'
All checks were successful
DCO action / DCO (pull_request) Successful in 1m2s
Details
Code generation / Generate wrappers (pull_request) Successful in 1m27s
Details
Tests / Tests (pull_request) Successful in 1m35s
Details
11d5b17bac 
Signed-off-by: Alexander Chuprov <a.chuprov@yadro.com>
fyrchik requested changes 2024-10-21 12:33:00 +00:00
Dismissed
common/address.go Outdated
@ -0,0 +1,36 @@
package common
func HexToBytes(s []byte) []byte {
fyrchik commented 2024-10-21 12:28:17 +00:00
Owner
Copy link

IMO having such low-level functions on a contract level is not worth it.
Do we need them?

IMO having such low-level functions on a contract level is not worth it. Do we _need_ them?
aarifullin commented 2024-10-21 13:30:14 +00:00
Member
Copy link

TBH, the usage of this function can barely be common (so, I agree with @fyrchik). The second point is that the input parameter seems irrelevant to the function's name - hex is the slice of bytes? I believe the function can be renamed or commented

TBH, the usage of this function can barely be common (so, I agree with @fyrchik). The second point is that the input parameter seems irrelevant to the function's name - hex is the slice of bytes? I believe the function can be renamed or commented
achuprov commented 2024-10-25 08:58:35 +00:00
Author
Member
Copy link

Fixed. Now using base 56.

Fixed. Now using base 56.
nns/frostfsid.go Outdated
@ -0,0 +16,4 @@
)
func checkFrostfsID(ctx storage.Context) bool {
frostfsIDAddress := getRecordsByType(ctx, []byte(tokenIDFromName(FrostfsIDNNSName)), FrostfsIDNNSName, TXT)
fyrchik commented 2024-10-21 12:29:43 +00:00
Owner
Copy link

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
fyrchik commented 2024-10-21 12:30:08 +00:00
Owner
Copy link

See #96

See https://git.frostfs.info/TrueCloudLab/frostfs-contract/issues/96
achuprov commented 2024-10-29 14:53:55 +00:00
Author
Member
Copy link

fixed

fixed
nns/frostfsid.go Outdated
@ -0,0 +24,4 @@
frostfsidAddr := common.ReverseBytes(common.HexToBytes([]byte(frostfsIDAddress[0])))
signers := runtime.CurrentSigners()
for _, signer := range signers {
if res := contract.Call(frostfsidAddr, "getValueForSubjectKey", contract.All, signer.Account, FrostfsIDNNSTLDPermissionKey).(string); res == FrostfsIDTLDRegistrationAllowed {
fyrchik commented 2024-10-21 12:31:38 +00:00
Owner
Copy link

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.
fyrchik commented 2024-10-21 12:32:00 +00:00
Owner
Copy link

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.
fyrchik commented 2024-10-21 12:32:53 +00:00
Owner
Copy link

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.
aarifullin commented 2024-10-21 14:21:10 +00:00
Member
Copy link

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?
fyrchik commented 2024-10-22 07:11:38 +00:00
Owner
Copy link

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).
👍 1
achuprov commented 2024-10-25 08:58:48 +00:00
Author
Member
Copy link

fixed

fixed
aarifullin reviewed 2024-10-21 13:46:58 +00:00
common/address.go Outdated
@ -0,0 +26,4 @@
}
}
func ReverseBytes(b []byte) []byte {
aarifullin commented 2024-10-21 13:46:58 +00:00
Member
Copy link

Please, leave a comment that a copied version of the input slice is being reversed in fact

Please, leave a comment that a copied version of the input slice is being reversed in fact
achuprov commented 2024-10-25 08:58:57 +00:00
Author
Member
Copy link

fixed

fixed
achuprov force-pushed feat/integrate_frostfsid2nns from 11d5b17bac to 7514f94daf 2024-10-25 08:08:33 +00:00 Compare
achuprov force-pushed feat/integrate_frostfsid2nns from 7514f94daf to b6983ae888 2024-10-25 08:09:09 +00:00 Compare
achuprov force-pushed feat/integrate_frostfsid2nns from b6983ae888 to 062998cc6b 2024-10-25 08:19:49 +00:00 Compare
achuprov force-pushed feat/integrate_frostfsid2nns from 062998cc6b to 1a2fc7dc60 2024-10-25 08:24:30 +00:00 Compare
fyrchik requested changes 2024-10-25 09:19:00 +00:00
Dismissed
tests/nns_test.go Outdated
@ -41,3 +59,3 @@
func TestNNSGeneric(t *testing.T) {
c := newNNSInvoker(t, false)
c, _ := newNNSInvoker(t, false, false)
fyrchik commented 2024-10-25 09:18:58 +00:00
Owner
Copy link

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 (!).
achuprov commented 2024-10-28 09:42:09 +00:00
Author
Member
Copy link

fixed

fixed
dkirillov reviewed 2024-10-25 13:52:25 +00:00
frostfsid/frostfsid_contract.go Outdated
@ -473,2 +473,4 @@
}
// GetValueForSubjectKey GetSubjectKey returns the value associated with the key for the subject.
func GetValueForSubjectKey(addr interop.Hash160, name string) string {
dkirillov commented 2024-10-25 13:52:25 +00:00
Member
Copy link

I suggest to rename this function to GetSubjectKV

I suggest to rename this function to `GetSubjectKV`
achuprov commented 2024-10-28 09:43:52 +00:00
Author
Member
Copy link

Renamed

Renamed
dkirillov marked this conversation as resolved
dkirillov reviewed 2024-10-25 13:55:57 +00:00
nns/frostfsid.go Outdated
@ -0,0 +22,4 @@
}
frostfsIDAddress := getRecordsByType(ctx, []byte(tokenIDFromName(FrostfsIDNNSName)), FrostfsIDNNSName, TXT)
if len(frostfsIDAddress) == 0 {
dkirillov commented 2024-10-25 13:55:57 +00:00
Member
Copy link

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]` ?
achuprov commented 2024-10-29 14:53:40 +00:00
Author
Member
Copy link

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.
dkirillov commented 2024-10-30 11:41:54 +00:00
Member
Copy link

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]`)
achuprov commented 2024-10-30 15:04:39 +00:00
Author
Member
Copy link

You're right. Fixed.

You're right. Fixed.
dkirillov commented 2024-10-31 06:33:32 +00:00
Member
Copy link

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

I'm not sure. It seems you fixed wrong line
dkirillov marked this conversation as resolved
dkirillov reviewed 2024-10-25 13:59:58 +00:00
rpcclient/frostfsid/client.go Outdated
@ -228,2 +228,4 @@
}
// GetValueForSubjectKey invokes `getValueForSubjectKey` method of contract.
func (c *ContractReader) GetValueForSubjectKey(addr util.Uint160, name string) (string, error) {
dkirillov commented 2024-10-25 13:59:58 +00:00
Member
Copy link

New method should also be added to frsotfsid/client/client.go file

New method should also be added to `frsotfsid/client/client.go` file
dkirillov marked this conversation as resolved
achuprov force-pushed feat/integrate_frostfsid2nns from 1a2fc7dc60 to 15c4f02500 2024-10-28 09:34:13 +00:00 Compare
achuprov force-pushed feat/integrate_frostfsid2nns from 15c4f02500 to cdc4e23eb2 2024-10-28 12:52:18 +00:00 Compare
dkirillov reviewed 2024-10-28 14:27:50 +00:00
frostfsid/client/client.go Outdated
@ -268,1 +269,4 @@
// GetSubjectKV invokes `getSubjectKV` method of contract.
func (c Client) GetSubjectKV(addr util.Uint160, name string) (string, error) {
value, err := unwrap.UTF8String(c.act.Call(c.contract, getSubjectKVMethod, addr, name))
dkirillov commented 2024-10-28 14:27:50 +00:00
Member
Copy link

It seems we can write just

return unwrap.UTF8String(c.act.Call(c.contract, getSubjectKVMethod, addr, name))
It seems we can write just ```golang return unwrap.UTF8String(c.act.Call(c.contract, getSubjectKVMethod, addr, name)) ```
achuprov commented 2024-10-29 14:49:32 +00:00
Author
Member
Copy link

fixed

fixed
dkirillov marked this conversation as resolved
achuprov force-pushed feat/integrate_frostfsid2nns from cdc4e23eb2 to b268eb5357 2024-10-29 14:43:47 +00:00 Compare
achuprov force-pushed feat/integrate_frostfsid2nns from b268eb5357 to 32508245c2 2024-10-29 14:47:41 +00:00 Compare
achuprov force-pushed feat/integrate_frostfsid2nns from 32508245c2 to 0d76a79d5b 2024-10-30 12:40:15 +00:00 Compare
fyrchik reviewed 2024-10-31 06:50:51 +00:00
nns/frostfsid.go Outdated
@ -0,0 +13,4 @@
const (
FrostfsIDNNSTLDPermissionKey = "nns-allow-register-tld"
FrostfsIDTLDRegistrationAllowed = "allow"
FrostfsIDTLDRegistrationDisallowed = "disallow"
fyrchik commented 2024-10-31 06:50:51 +00:00
Owner
Copy link

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.
fyrchik approved these changes 2024-10-31 06:50:55 +00:00
Dismissed
achuprov force-pushed feat/integrate_frostfsid2nns from 0d76a79d5b to 4881a8a46c 2024-11-01 07:42:03 +00:00 Compare
achuprov dismissed fyrchik's review 2024-11-01 07:42:03 +00:00
Reason:

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

fyrchik approved these changes 2024-11-01 08:58:14 +00:00
Dismissed
aarifullin approved these changes 2024-11-02 07:13:34 +00:00
Dismissed
achuprov force-pushed feat/integrate_frostfsid2nns from 4881a8a46c to 48f06df25a 2024-11-02 08:25:18 +00:00 Compare
achuprov dismissed fyrchik's review 2024-11-02 08:25:18 +00:00
Reason:

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

achuprov dismissed aarifullin's review 2024-11-02 08:25:18 +00:00
Reason:

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

dkirillov approved these changes 2024-11-05 06:53:37 +00:00
dkirillov left a comment
Member
Copy link

LGTM

LGTM
achuprov referenced this pull request from a commit 2024-11-05 10:57:06 +00:00
[#119] frostfsid: Add 'getSubjectKV'
fyrchik merged commit 48f06df25a into master 2024-11-05 14:58:57 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
dkirillov
Labels
Clear labels   
P0

Highest

  
P1

High

  
P2

Medium

  
P3

Low

  
good first issue

Good for newcomers

  
Infrastructure

Infrastructure, Automation, CI/CD or DevOps related stuff

  
blocked

The issue or PR is blocked by something

  
bug

Something is not working

  
config

Configuration format update or breaking change

  
discussion

Talking about something in order to reach a decision or to exchange ideas

  
documentation

Documentation related

  
duplicate

This issue or pull request already exists

  
enhancement

New feature

  
go

Language features adoption

  
help wanted

Extra attention is needed

  
internal

   
invalid

Something is wrong

  
kludge

This is a kludge and we know it

  
observability

Related to metrics, tracing and logs

  
perfomance

Perfomance related

  
question

More information is needed

  
refactoring

Refactoring

  
wontfix

This won't be fixed

No labels
P0
P1
P2
P3
good first issue
Infrastructure
blocked
bug
config
discussion
documentation
duplicate
enhancement
go
help wanted
internal
invalid
kludge
observability
perfomance
question
refactoring
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
4 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-contract#119
No description provided.
Delete branch "achuprov/frostfs-contract:feat/integrate_frostfsid2nns"

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?