[#48] frostfsid: Update contract #40

dkirillov · 2023-10-02T06:45:44Z

dkirillov commented

2023-10-02 06:45:44 +00:00

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

frostfsid/frostfsid_contract.go Outdated

					
				@ -160,0 +710,4 @@

				}

				func namespaceKey(ns string) []byte {

					return namespaceKeyFromHash(ripemd160Hash(ns))

fyrchik commented

2023-10-17 07:26:04 +00:00

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

dkirillov commented

2023-10-17 07:40:40 +00:00

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      |

fyrchik commented

2023-10-24 06:57:29 +00:00

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

dkirillov commented

2023-10-24 12:27:36 +00:00

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

alexvanin commented

2023-11-08 07:58:16 +00:00

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                                     |

fyrchik commented

2023-10-24 06:53:51 +00:00

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

dkirillov commented

2023-10-24 12:34:06 +00:00

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

frostfsid/frostfsid_contract.go Outdated

					
				@ -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 {

fyrchik commented

2023-10-24 06:55:44 +00:00

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?

dkirillov commented

2023-10-24 12:37:27 +00:00

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

frostfsid/frostfsid_contract.go Outdated

					
				@ -160,0 +173,4 @@

						panic("address not found")

					}

					subject := std.Deserialize(data).(Subject)

					subject.AdditionalKeys = append(subject.AdditionalKeys, key)

fyrchik commented

2023-10-24 07:10:16 +00:00

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.

dkirillov commented

2023-10-24 12:42:54 +00:00

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

commonclient/invoker.go Outdated

					
				@ -0,0 +1,20 @@

				package commonclient

fyrchik commented

2023-10-24 16:39:42 +00:00

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.

fyrchik commented

2023-10-24 16:52:45 +00:00

#47

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

dkirillov commented

2023-10-25 06:36:22 +00:00

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

dkirillov commented

2023-11-07 11:10:05 +00:00

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

fyrchik referenced this pull request from TrueCloudLab/frostfs-node

2023-10-25 12:22:32 +00:00

Support new frostfs ID contract in IR #755

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

frostfsid/frostfsid_contract.go Outdated

					
				@ -160,0 +130,4 @@

					addr := contract.CreateStandardAccount(key)

					sKey := subjectKeyFromAddr(addr)

					data := storage.Get(ctx, sKey).([]byte)

					if data != nil {

aarifullin commented

2023-11-08 06:51:22 +00:00

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*/ } ```

fyrchik commented

2023-11-08 07:22:06 +00:00

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)

aarifullin commented

2023-11-09 15:00:08 +00:00

Ah, okay! Thanks

aarifullin marked this conversation as resolved

aarifullin approved these changes 2023-11-08 06:56:50 +00:00

alexvanin commented

2023-11-08 08:00:39 +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

👍 1

fyrchik commented

2023-11-08 13:40:57 +00:00

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

[#48 ] frostfsid: Update storage scheme doc

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

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 referenced this pull request from TrueCloudLab/frostfs-node

2023-11-13 12:23:49 +00:00

adm: Support new FrostFS ID contract #793

fyrchik approved these changes 2023-11-14 09:45:39 +00:00

frostfsid/frostfsid_contract.go Outdated

					
				@ -57,0 +98,4 @@

					storage.Delete(ctx, ownerKey(addr))

				}

				func ListOwners() iterator.Iterator {

fyrchik commented

2023-10-31 10:13:55 +00:00

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?

dkirillov commented

2023-11-14 11:11:10 +00:00

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

fyrchik commented

2023-11-14 11:43:29 +00:00

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