Why dBFT requires NeoGO package

[im-kulikov]

It's strange to understand that the dBFT package requires NeoGo, which requires dBFT.

I propose to move dependencies into the dBFT package.

That adds the possibility to use the package separately in other projects without NeoGo requirements.

cc @fyrchik @realloc @anatoly-bogatyrev @roman-khimov

[roman-khimov]

These are all modules, it makes no sense to shuffle them around. Just use modules from the origin repository that developed them.

That adds the possibility to use the package separately in other projects without NeoGo requirements.

I don't think this potential move would change anything.

[im-kulikov]

I don't think this potential move would change anything.

That adds a possibility to ignore NeoGO mono repo as a dependency.

The dBFT used two types from NeoGO and only for that it requires it. That sounds strange.

[im-kulikov]

My point is in moving or rewrites that types into dBFT and remove unnecessary dependencies.

[roman-khimov]

That adds a possibility to ignore NeoGO mono repo as a dependency.

And that gives us like... what, really? It's not a linux repository, it's like 18M repository.

The dBFT used two types from NeoGO and only for that it requires it. That sounds strange.

Not so much, it reuses module that it needs to and that's it.

My point is in moving or rewrites that types into dBFT and remove unnecessary dependencies.

My point is that moving code between repositories completely destroys changes history and I just hate that.

And rewriting is like... please, don't. There is a module that implements exactly what you need, hoooray! just use it.

[im-kulikov]

Not so much, it reuses module that it needs to and that's it.

Nope. It adds a lot of unnecessary dependencies from NeoGO module.

And that gives us like... what, really? It's not a Linux repository, it's like 18M repository.
Not so much, it reuses module that it needs to and that's it.

For two types I should import 18MB? It sounds ugly.

[im-kulikov]

It was assumed that part of the NeoGO package (pkg) will be allocated to separate repositories. Now we are turning NeoGO into a mono repo. What then was the point of putting dBFT as a separate repository, if it still requires NeoGO?

[roman-khimov]

Nope. It adds a lot of unnecessary dependencies from NeoGO module.

Like io module? That's the only one being added. And to be fair it's only added to satisfy data type, no code should be pulled from it.

For two types I should import 18MB? It sounds ugly.

That's git repository size, I don't think anyone cares about that. But you only really add one tiny module to the executable and that's it.

[roman-khimov]

After discussion with @im-kulikov we've settled upon abstracting away this types with interfaces if there is a need to really separate this two repositories.

But looking at how these types are used (throughout the whole repository) it probably won't be that easy.

[im-kulikov]

Yep. An interface would be a great decision.

[fyrchik]

What then was the point of putting dBFT as a separate repository, if it still requires NeoGO?

@im-kulikov to be able to use it from both neo-go and neofs.

[fyrchik]

Well, we can probably hide util.Uint* behind something like interface { Equal(..) bool } and require making wrappers in client, because function type can't be a subtype of another type.

But io package from neo-go simplifies marshalling a lot and makes it a lot more readable. It is used mostly for default implementations and we can get rid of it but then require client to implement all possible payloads. I'd rather prefer to be able to use a library that "just works" in simple cases and at the same time is rather flexible to be used in more picky situations.

[im-kulikov]

But io package from neo-go simplifies marshalling a lot and makes it a lot more readable.

It's just syntactic sugar and I don't see any problems to use the binary package as is.

[im-kulikov]

Well, we can probably hide util.Uint* behind something like interface { Equal(..) bool } and require making wrappers in client, because function type can't be a subtype of another type.

As we discuss with @roman-khimov, there is ok to use Hasher interface and returns the slice of bytes ([]byte). On the usage side, you can wrap it with an internal type as you want.

[roman-khimov]

I don't think we'll ever untie these.

[fyfyrchik]

I have cleaned modcache and after this I have:

$ go env GOPROXY
direct

$ go test -count=1 ./...
go: downloading github.com/nspcc-dev/neo-go v0.73.1-pre.0.20200303142215-f5a1b928ce09
crypto/hash.go:6:2: github.com/nspcc-dev/neo-go@v0.73.1-pre.0.20200303142215-f5a1b928ce09: invalid pseudo-version: preceding tag (v0.73.1-pre) not found
block/block.go:6:2: github.com/nspcc-dev/neo-go@v0.73.1-pre.0.20200303142215-f5a1b928ce09: invalid pseudo-version: preceding tag (v0.73.1-pre) not found

$ GOPROXY=proxy.golang.org go test -count=1 ./...
?       github.com/nspcc-dev/dbft/simulation    [no test files]
ok      github.com/nspcc-dev/dbft       0.007s
ok      github.com/nspcc-dev/dbft/block 0.003s
ok      github.com/nspcc-dev/dbft/crypto        0.003s
ok      github.com/nspcc-dev/dbft/merkle        0.003s
ok      github.com/nspcc-dev/dbft/payload       0.003s
ok      github.com/nspcc-dev/dbft/timer 1.304s

Makes me appreciate what Go team has done regarding versioning, though.

[roman-khimov]

This can be useful for Bane, btw. As suggested in #87 the only real problem is util.Uint* types. Coincidentally, it'd be easier for us to use different native types there (avoiding conversion we have currently).