[#48] frostfsid: Update contract #40

Merged
fyrchik merged 9 commits from dkirillov/frostfs-contract:poc/frostfsid into master 2023-11-14 11:43:48 +00:00
Collaborator

close #48

close #48
dkirillov self-assigned this 2023-10-02 06:45:44 +00:00
dkirillov force-pushed poc/frostfsid from 16773086a1 to 229ce056c3 2023-10-02 11:14:17 +00:00 Compare
fyrchik reviewed 2023-10-17 07:26:04 +00:00
@ -160,0 +710,4 @@
}
func namespaceKey(ns string) []byte {
return namespaceKeyFromHash(ripemd160Hash(ns))

I suppose we use hashes because we want to allow strings of arbitrary lengths?

I suppose we use hashes because we want to allow strings of arbitrary lengths?
Poster
Collaborator

Yes.

Yes.
fyrchik marked this conversation as resolved
fyrchik reviewed 2023-10-24 07:12:54 +00:00
frostfsid/doc.go Outdated
@ -26,1 +11,3 @@
| `o` + ownerID + publicKey | ByteArray | it flags owner's public key |
| Key | Value | Description |
|---------------------------------------------------------------------------------|--------------------------------|----------------------------------------------------|
| `o` + [ owner address ] | []byte{1} | contract owners that can invoke write methods |

What kind of "owners" can be here besides the committee?

What kind of "owners" can be here besides the committee?
Poster
Collaborator

In case of TO this can be CM or IAM. But we have not decided yet should it be someone besides the committee @alexvanin

In case of TO this can be CM or IAM. But we have not decided yet should it be someone besides the committee @alexvanin

We want to have external validators. They can validate email, phone number or any other claim user may have. It isn't committee work to validate and approve such things.

These validators can create multi-signed transaction and multiaddress can be added as owner. These owners managed by committee \ consensus.

In trusted networks, validator key addresses can be added as owner, for example IAM service instances.

We want to have external validators. They can validate email, phone number or any other claim user may have. It isn't committee work to validate and approve such things. These validators can create multi-signed transaction and multiaddress can be added as owner. These owners managed by committee \ consensus. In trusted networks, validator key addresses can be added as owner, for example IAM service instances.
frostfsid/doc.go Outdated
@ -27,0 +13,4 @@
| `o` + [ owner address ] | []byte{1} | contract owners that can invoke write methods |
| `s` + [ subject address ] | Serialized Subject structure | subject into |
| `a` + [ pk address ] + [ subject address ] | []byte{1} | link extra public keys for subject |
| `n` + [ RIPEMD160 of namespace ] | Serialized Namespace structure | namespace info |

Is there any reason we use RIPEMD instead of SHA256 which is also supported by the VM?

Is there any reason we use RIPEMD instead of SHA256 which is also supported by the VM?
Poster
Collaborator

specifically for this key there isn't any reason besides consistency and possibility reusing already computed ns hash for other keys (but currently as I see we don't do this)

specifically for this key there isn't any reason besides consistency and possibility reusing already computed ns hash for other keys (but currently as I see we don't do this)
fyrchik marked this conversation as resolved
@ -47,2 +73,2 @@
if len(args.addrNetmap) != interop.Hash160Len || len(args.addrContainer) != interop.Hash160Len {
panic("incorrect length of contract script hash")
for _, owner := range args.owners {
if len(owner) != interop.Hash160Len {

In some places ownerID is 25 bytes (+ 1-byte prefix + 4-byte checksum), in others just neo address.
I like 20-bytes better, but could this cause problems?

In some places `ownerID` is 25 bytes (+ 1-byte prefix + 4-byte checksum), in others just neo address. I like 20-bytes better, but could this cause problems?
Poster
Collaborator

I'm not sure. I think we can require use 20-bytes address. We also provide client to interact with contract that make client use the addresses that we expect

I'm not sure. I think we can require use 20-bytes address. We also provide client to interact with contract that make client use the addresses that we expect
fyrchik marked this conversation as resolved
@ -160,0 +173,4 @@
panic("address not found")
}
subject := std.Deserialize(data).(Subject)
subject.AdditionalKeys = append(subject.AdditionalKeys, key)

This can cause conflict when multiple keys are added at the same time: array serialization is done with varuint
Not sure my concern is practical, though.

This can cause conflict when multiple keys are added at the same time: array serialization is done with `varuint` Not sure my concern is practical, though.
Poster
Collaborator

We can add multiple keys in the same transaction (isn't this enough to cover use-cases?):


func TestFrostFSID_Client_TestAddMultipleKeys(t *testing.T) {
	ffsid, cancel := initFrostfsIFClientTest(t)
	defer cancel()

	dataUserKey, dataUserAddr := newKey(t)
	extraDataUserKey, _ := newKey(t)
	extraDataUserKey2, _ := newKey(t)

	tx := ffsid.cli.StartTx()
	err := tx.WrapCall(ffsid.cli.CreateSubjectCall(dataUserKey.PublicKey()))
	require.NoError(t, err)
	err = tx.WrapCall(ffsid.cli.AddSubjectKeyCall(dataUserAddr, extraDataUserKey.PublicKey()))
	require.NoError(t, err)
	err = tx.WrapCall(ffsid.cli.AddSubjectKeyCall(dataUserAddr, extraDataUserKey2.PublicKey()))
	require.NoError(t, err)
	ffsid.a.await(ffsid.cli.SendTx(tx))

	subj, err := ffsid.cli.GetSubject(dataUserAddr)
	require.NoError(t, err)
	require.ElementsMatch(t, keys.PublicKeys{extraDataUserKey.PublicKey(), extraDataUserKey2.PublicKey()}, subj.AdditionalKeys)
}
We can add multiple keys in the same transaction (isn't this enough to cover use-cases?): ``` func TestFrostFSID_Client_TestAddMultipleKeys(t *testing.T) { ffsid, cancel := initFrostfsIFClientTest(t) defer cancel() dataUserKey, dataUserAddr := newKey(t) extraDataUserKey, _ := newKey(t) extraDataUserKey2, _ := newKey(t) tx := ffsid.cli.StartTx() err := tx.WrapCall(ffsid.cli.CreateSubjectCall(dataUserKey.PublicKey())) require.NoError(t, err) err = tx.WrapCall(ffsid.cli.AddSubjectKeyCall(dataUserAddr, extraDataUserKey.PublicKey())) require.NoError(t, err) err = tx.WrapCall(ffsid.cli.AddSubjectKeyCall(dataUserAddr, extraDataUserKey2.PublicKey())) require.NoError(t, err) ffsid.a.await(ffsid.cli.SendTx(tx)) subj, err := ffsid.cli.GetSubject(dataUserAddr) require.NoError(t, err) require.ElementsMatch(t, keys.PublicKeys{extraDataUserKey.PublicKey(), extraDataUserKey2.PublicKey()}, subj.AdditionalKeys) } ```
fyrchik marked this conversation as resolved
fyrchik reviewed 2023-10-24 16:39:42 +00:00
@ -0,0 +1,20 @@
package commonclient

What about using neo-go contract generate-rpcwrapper?

neo-go contract generate-rpcwrapper --manifest frostfsid/config.json --config frostfsid/config.yml --out ./clients/frostfsid/client.go

It generates actor-friendly interface together with all wrappers, even event parsers.

What about using `neo-go contract generate-rpcwrapper`? ``` neo-go contract generate-rpcwrapper --manifest frostfsid/config.json --config frostfsid/config.yml --out ./clients/frostfsid/client.go ``` It generates actor-friendly interface together with all wrappers, even event parsers.

#47

https://git.frostfs.info/TrueCloudLab/frostfs-contract/pulls/47
Poster
Collaborator

Hm... Looks good but as I understand we want more user-friendly client so we will have to add some wrappers for generated client. Thus I'm not sure if it worth to rewrite code right now. Probably we can update it a little bit later, after #47 be merged and when we open this PR for review

Hm... Looks good but as I understand we want more user-friendly client so we will have to add some wrappers for generated client. Thus I'm not sure if it worth to rewrite code right now. Probably we can update it a little bit later, after #47 be merged and when we open this PR for review
Poster
Collaborator

I'm not sure if it makes sense to use generated wrappers in frostfsid/client package. Because of:

  • We want to get result.Invoke in functions like this but generated wrapper doesn't provide such thing
  • We want to be able to invoke several method in one tx but generated wrappers has not convenient way to get script for that purpose
I'm not sure if it makes sense to use generated wrappers in `frostfsid/client` package. Because of: * We want to get `result.Invoke` in functions like [this](https://git.frostfs.info/dkirillov/frostfs-contract/src/commit/2af51ee8f43f84e6cb17af77f3bcaf9be7204f64/frostfsid/client/client.go#L194) but generated wrapper [doesn't provide](https://git.frostfs.info/dkirillov/frostfs-contract/src/commit/2af51ee8f43f84e6cb17af77f3bcaf9be7204f64/rpcclient/frostfsid/client.go#L479-L499) such thing * We want to be able to invoke several method in [one tx](https://git.frostfs.info/dkirillov/frostfs-contract/src/commit/2af51ee8f43f84e6cb17af77f3bcaf9be7204f64/commonclient/transaction.go#L35) but generated wrappers has not convenient way to get script for that purpose
dkirillov force-pushed poc/frostfsid from 7b4207f779 to 2af51ee8f4 2023-11-07 11:02:48 +00:00 Compare
dkirillov changed title from WIP: [#XX] frostfsid: Update contract to [#XX] frostfsid: Update contract 2023-11-07 11:28:14 +00:00
dkirillov requested review from storage-core-committers 2023-11-07 11:28:23 +00:00
dkirillov requested review from storage-core-developers 2023-11-07 11:28:24 +00:00
dkirillov requested review from storage-services-committers 2023-11-07 11:28:24 +00:00
dkirillov requested review from storage-services-developers 2023-11-07 11:28:24 +00:00
dkirillov changed title from [#XX] frostfsid: Update contract to [#48] frostfsid: Update contract 2023-11-07 11:29:22 +00:00
dstepanov-yadro approved these changes 2023-11-07 13:26:01 +00:00
acid-ant approved these changes 2023-11-07 15:51:18 +00:00
aarifullin reviewed 2023-11-08 06:51:22 +00:00
@ -160,0 +130,4 @@
addr := contract.CreateStandardAccount(key)
sKey := subjectKeyFromAddr(addr)
data := storage.Get(ctx, sKey).([]byte)
if data != nil {
Collaborator

Would not be better to check with if len(data) > 0?
I know this barely may happen but

data := []byte{}
if data != nil {  /*that's true*/
}
Would not be better to check with `if len(data) > 0`? I know this barely may happen but ```golang data := []byte{} if data != nil { /*that's true*/ } ```

I believe this nil is actually Null, so len check will panic (and it costs more anyway)

I believe this nil is actually `Null`, so `len` check will panic (and it costs more anyway)
Collaborator

Ah, okay! Thanks

Ah, okay! Thanks
aarifullin marked this conversation as resolved
aarifullin approved these changes 2023-11-08 06:56:50 +00:00

@pogpp has some question for group related operations. I suggest to address them in this PR and then merge it.
/cc @dkirillov

@pogpp has some question for group related operations. I suggest to address them in this PR and then merge it. /cc @dkirillov

frostfs-adm support PR TrueCloudLab/frostfs-node#793

frostfs-adm support PR https://git.frostfs.info/TrueCloudLab/frostfs-node/pulls/793
dkirillov force-pushed poc/frostfsid from c830abacad to 80fd9d593e 2023-11-09 08:01:35 +00:00 Compare
dkirillov force-pushed poc/frostfsid from 80fd9d593e to e2e406932f 2023-11-09 13:14:25 +00:00 Compare
dkirillov added 1 commit 2023-11-09 14:39:35 +00:00
DCO action / DCO (pull_request) Successful in 58s Details
Tests / Tests (1.19) (pull_request) Successful in 1m37s Details
Tests / Tests (1.20) (pull_request) Successful in 1m32s Details
dd5919348d
[#48] frostfsid: Update storage scheme doc
Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>
aarifullin approved these changes 2023-11-09 15:00:29 +00:00
pogpp approved these changes 2023-11-13 10:59:35 +00:00
fyrchik approved these changes 2023-11-14 09:45:39 +00:00
@ -57,0 +98,4 @@
storage.Delete(ctx, ownerKey(addr))
}
func ListOwners() iterator.Iterator {

As I understand, the list of owners is small, almost certainly less than 10.
In this case returning an array seems a better option, what do you think?

As I understand, the list of owners is small, almost certainly less than 10. In this case returning an array seems a better option, what do you think?
Poster
Collaborator

Will we have 100 owners in private 100-node installation?

Will we have 100 owners in private 100-node installation?

Well, it depends -- multisig 1 of N is also an option.

Well, it depends -- multisig 1 of N is also an option.
fyrchik marked this conversation as resolved
fyrchik approved these changes 2023-11-14 09:47:26 +00:00
fyrchik merged commit dd5919348d into master 2023-11-14 11:43:48 +00:00
Sign in to join this conversation.
No reviewers
TrueCloudLab/storage-services-committers
No Milestone
No Assignees
7 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#40
There is no content yet.