Drop most of deploy configs #370

roman-khimov · 2023-11-23T19:59:44Z

No description provided.

cthulhu-rider · 2023-11-24T08:42:45Z

neofsid/neofsid_contract.go

@@ -47,25 +46,23 @@ func _deploy(data any, isUpdate bool) {
 			switchToNotary(ctx)
 		}

+		// netmap is not used for quite some time and deleted in 0.19.0.
+		if version < 19_000 {
+			storage.Delete(ctx, "netmapScriptHash")


lets check this in migration test

cthulhu-rider · 2023-11-24T08:49:37Z

alphabet/alphabet_contract.go

-	if len(args.addrNetmap) != interop.Hash160Len || len(args.addrProxy) != interop.Hash160Len {
-		panic("incorrect length of contract script hash")
+	if len(args.addrNetmap) != interop.Hash160Len {
+		args.addrNetmap = common.ResolveFSContract("netmap")


this looks OK if len(args.addrNetmap) == 0, otherwise, if it is not interop.Hash160Len, i suggest to panic (set correct address or completely omit, do not pass garbage)

The behavior now is to derive address from NNS, so in most cases it will just get a proper hash. In the future non-NNS logic can be omitted completely. I'd leave it as is for now, tools that care pass correct things and we resolve from NNS otherwise.

i would say that check is better for me too. but if we know that it is kinda overriding now and NNS gonna be the only true way soon, i am OK with this change

cthulhu-rider · 2023-11-24T08:55:00Z

common/nns.go

+
+// ResolveFSContract returns contract hash by name as registered in NNS or
+// panics if it can't be resolved.
+func ResolveFSContract(name string) interop.Hash160 {


nice candidate to be tested

It is tested in deployment scenarios.

fault conditions too? E.g. hello world instead of hash

It's not a part of any public contract API, so a unit-test would require creating a specific contract to test just this thing. Push garbage in the deployment tests... Can be done, but I doubt it's useful. "hello world" is to be tried as an address and it will fail decoding miserably.

cthulhu-rider · 2023-11-24T08:58:38Z

container/container_contract.go

+		args.addrNNS = common.InferNNSHash()
+	}
+	if len(args.addrNetmap) != interop.Hash160Len {
+		args.addrNetmap = common.ResolveFSContract("netmap")


we've already resolved args.addrNNS and do this again in each common.ResolveFSContract. I'd add func common.ResolveNeoFSContractWithNNS(addrNNS, name) and call it in common.ResolveNeoFSContract and here

I'm not sure I like the resulting code, but OK.

cthulhu-rider · 2023-11-24T09:05:33Z

container/container_contract.go

@@ -166,6 +169,9 @@ func _deploy(data any, isUpdate bool) {
 	if len(args.addrID) != interop.Hash160Len {
 		args.addrID = common.ResolveFSContract("neofsid")
 	}
+	if len(args.nnsRoot) == 0 {
+		args.nnsRoot = nnsDefaultTLD


this should be documented somewhere

cthulhu-rider · 2023-11-24T09:09:12Z

tests/container_test.go

-		addrNetmap, addrBalance, addrNNS = &ctrNetmap.Hash, &ctrBalance.Hash, &nnsHash
+		deployContainerContract(t, e, &ctrNetmap.Hash, &ctrBalance.Hash, &nnsHash)
+	} else {
+		_ = deployNeoFSIDContract(t, e) // Needed in this case to autoresolve.


easy to forget, so lets do inside deployContainerContractInt or at least write comment

deployContainerContractInt deploys container only, it doesn't care about dependencies. Dependencies are handled here.

this doesnt change my request

what Int stays for in deployContainerContractInt? Internal? Integer?

cthulhu-rider · 2023-11-24T09:11:12Z

common/nns.go

+// ContractTLD is the default domain used by NeoFS contracts.
+const ContractTLD = "neofs"
+
+// InferNNSHash returns NNS contract hash by its well-known ID or panics if


Suggested change

// InferNNSHash returns NNS contract hash by its well-known ID or panics if

// InferNNSHash returns NNS contract hash by [NNSID] or panics if

it's not well-known enough

common/nns.go

neofsid/neofsid_contract.go

Signed-off-by: Roman Khimov <roman@nspcc.ru>

Proxy doesn't need them and it doesn't have any deploy parameters. Signed-off-by: Roman Khimov <roman@nspcc.ru>

Signed-off-by: Roman Khimov <roman@nspcc.ru>

Make them somewhat closer in style to RPC bindings. Signed-off-by: Roman Khimov <roman@nspcc.ru>

They're still there to override/keep compatibility, but they're not mandatory now. Closes #325. Signed-off-by: Roman Khimov <roman@nspcc.ru>

Many contracts no longer have any deploy flags and while we've kept them for update during transition this is no longer needed, there is no non-notary mode. Make them completely parameterless. Signed-off-by: Roman Khimov <roman@nspcc.ru>

They're no longer needed. Signed-off-by: Roman Khimov <roman@nspcc.ru>

There is a nice well-known default. Signed-off-by: Roman Khimov <roman@nspcc.ru>

These still have parameters, but we can forget about the notary flag. Signed-off-by: Roman Khimov <roman@nspcc.ru>

Signed-off-by: Roman Khimov <roman@nspcc.ru>

We have a relatively stable set of configs now and this is just not needed, updates can and should work with an empty config. Signed-off-by: Roman Khimov <roman@nspcc.ru>

Don't try interpreting data when there is no need to. Signed-off-by: Roman Khimov <roman@nspcc.ru>

41e483f was a bit more correct, we can't use std.Atoi since it's intended for C#-compatible integers which means they will be interpreted as integers for input and output (including signedness) and the resulting byte set can differ from the input one leading to failed calls. Signed-off-by: Roman Khimov <roman@nspcc.ru>

tests/neofsid_test.go

carpawell · 2023-11-28T13:22:21Z

common/nns.go

+// Contract name should be lowercased, should not include [ContractTLD]. Example
+// values: "netmap", "container", etc. Relies on NeoFS-specific NNS setup, see
+// [NNSID].
+func ResolveFSContract(contractName string) interop.Hash160 {


what does FS stand for here?

but there is no Neo in the method's name. i can understand and accept it but do not think it is a good name

common/nns.go

carpawell · 2023-11-28T13:26:11Z

alphabet/alphabet_contract.go

-	if len(args.addrNetmap) != interop.Hash160Len || len(args.addrProxy) != interop.Hash160Len {
-		panic("incorrect length of contract script hash")
+	if len(args.addrNetmap) != interop.Hash160Len {
+		args.addrNetmap = common.ResolveFSContract("netmap")


i would say that check is better for me too. but if we know that it is kinda overriding now and NNS gonna be the only true way soon, i am OK with this change

tests/container_test.go

netmap/netmap_contract.go

common/nns.go

roman-khimov requested a review from AnnaShaleva November 23, 2023 19:59

roman-khimov requested review from carpawell and cthulhu-rider as code owners November 23, 2023 19:59

roman-khimov force-pushed the drop-deploy-configs branch 2 times, most recently from d2cfc28 to 35d6ac3 Compare November 24, 2023 09:09

cthulhu-rider reviewed Nov 24, 2023

View reviewed changes

roman-khimov force-pushed the drop-deploy-configs branch from 35d6ac3 to a9271a3 Compare November 24, 2023 09:46

roman-khimov requested a review from cthulhu-rider November 24, 2023 09:46

AnnaShaleva reviewed Nov 27, 2023

View reviewed changes

common/nns.go Show resolved Hide resolved

common/nns.go Outdated Show resolved Hide resolved

neofsid/neofsid_contract.go Show resolved Hide resolved

roman-khimov force-pushed the drop-deploy-configs branch 2 times, most recently from deada05 to 5e2a9cb Compare November 27, 2023 21:42

roman-khimov added 15 commits November 28, 2023 00:43

neofsid: drop unused netmap container address

400ce83

Signed-off-by: Roman Khimov <roman@nspcc.ru>

proxy: don't deploy useless contracts for tests

d758c0f

Proxy doesn't need them and it doesn't have any deploy parameters. Signed-off-by: Roman Khimov <roman@nspcc.ru>

tests: make a const for the default container domain

603a1d6

Signed-off-by: Roman Khimov <roman@nspcc.ru>

common: refactor nns getters slightly

9392a61

Make them somewhat closer in style to RPC bindings. Signed-off-by: Roman Khimov <roman@nspcc.ru>

*: make address params optional, take them from NNS

79cd25f

They're still there to override/keep compatibility, but they're not mandatory now. Closes #325. Signed-off-by: Roman Khimov <roman@nspcc.ru>

neofsid: drop deploy/update parameters

eea544e

They're no longer needed. Signed-off-by: Roman Khimov <roman@nspcc.ru>

container: make TLD parameter optional

5bcb74d

There is a nice well-known default. Signed-off-by: Roman Khimov <roman@nspcc.ru>

*: do not look into notary flag in remaining contracts

ca21ab6

These still have parameters, but we can forget about the notary flag. Signed-off-by: Roman Khimov <roman@nspcc.ru>

container: make parameters completely optional

7884c0b

Signed-off-by: Roman Khimov <roman@nspcc.ru>

netmap: mark keys deploy parameter as unused

3e16263

Signed-off-by: Roman Khimov <roman@nspcc.ru>

container: micro-optimize deploy GAS cost by NNS reuse

bc02bf7

Signed-off-by: Roman Khimov <roman@nspcc.ru>

netmap: never ever mess with config on update

ed1b60f

We have a relatively stable set of configs now and this is just not needed, updates can and should work with an empty config. Signed-off-by: Roman Khimov <roman@nspcc.ru>

netmap: allow to update with no parameters at all

cf06fcc

Don't try interpreting data when there is no need to. Signed-off-by: Roman Khimov <roman@nspcc.ru>

roman-khimov force-pushed the drop-deploy-configs branch from 5e2a9cb to b8745c8 Compare November 27, 2023 21:43

carpawell reviewed Nov 28, 2023

View reviewed changes

roman-khimov closed this Nov 28, 2023

roman-khimov deleted the drop-deploy-configs branch November 28, 2023 14:51

roman-khimov mentioned this pull request Nov 28, 2023

Drop deploy configs #373

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drop most of deploy configs #370

Drop most of deploy configs #370

roman-khimov commented Nov 23, 2023

cthulhu-rider Nov 24, 2023

roman-khimov Nov 24, 2023

cthulhu-rider Nov 24, 2023

roman-khimov Nov 24, 2023

carpawell Nov 28, 2023

cthulhu-rider Nov 24, 2023

roman-khimov Nov 24, 2023

cthulhu-rider Nov 24, 2023

roman-khimov Nov 24, 2023

cthulhu-rider Nov 24, 2023

roman-khimov Nov 24, 2023

cthulhu-rider Nov 24, 2023

cthulhu-rider Nov 24, 2023

roman-khimov Nov 24, 2023

cthulhu-rider Nov 24, 2023

roman-khimov Nov 24, 2023

cthulhu-rider Nov 24, 2023

carpawell Nov 28, 2023

roman-khimov Nov 28, 2023

carpawell Nov 28, 2023

carpawell Nov 28, 2023

	// InferNNSHash returns NNS contract hash by its well-known ID or panics if
	// InferNNSHash returns NNS contract hash by [NNSID] or panics if

Drop most of deploy configs #370

Drop most of deploy configs #370

Conversation

roman-khimov commented Nov 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment