perun-network / go-perun_archived Goto Github PK

Current State  The db.ChanIterator is a pair of channels: One for db.Item values and one for errors. It is used as a simpler representation of an iterator. db.IteratorChanWrapper seems to be a default implementation of the interface. It is created using db.AsChanIterator(Iterator).

Problem  The name is awkward, as ChanIterator suggests that the type itself is an iterator (which it is not, it is a pair of channels). It also suggests that it iterates over a list of channels, which it is not, either. Additionally, IteratorChanWrapper is inconsistently named with ChanIterator. AsChanIterator feels awkward.

Improvement Proposal  I suggest that the interface be named more descriptive of what it is, i.e. "Response", "ItemChannel", "Items". I also suggest that IteratorChanWrapper be named Default* or *Impl (where * is one of the previous names), as it seems to me that it is simply a default implementation of the interface. Default*/*Impl seems more descriptive to me than *Wrapper, because it actually wraps around the db.Iterator type, not around the IteratorChan type. I suggest AsChanIterator be made a member of the Iterator interface, and be renamed to To* or As* (i.e., ToItemChannel, ToItems). This coupling makes sense to me, as it is easier to read when used, and I think it is the predominant way in which the Iterator interface is read from anyway.

Current state  Currently, the type common.Address is just a generic implementation using a byte array, and utilities for conversion to/from hex strings.

Problem  This may prove insufficient, as not every byte array is necessarily a valid address for a given cryptocurrency, and some cryptocurrencies might display addresses in base58 or similar, which would lead to confusion if perun didn't support that representation.

Improvement Proposal  I propose that we use a more differentiated interface, which allows individual cryptocurrencies to have a better-fitting data model. Additionally, I suggest we provide a default generic implementation, which is similar to the current one, so that lazy developers can use addresses without implementation overhead.
I suggest the common.Address interface contains:

• ToString() string: Human-readable string (as employed by the cryptocurrency it belongs to, might be hex, base58, …).
• FromString(string) (*Address, error): Parsing of a human-readable representation.
• FromBytes([]byte) (*Address, error): Simple raw deserialisation.
• ToBytes() []byte: Simple raw serialisation.

Additionally, it might be of interest to additionally store which domain/blockchain an address belongs to, in order to make debugging and error checking easier.
I suggest the default implementation be named common.GenericAddress, and consist of a byte array, and represents addresses as hex strings.

Testing & CI/CD is an integral part of our workflow.

Current state  We do not have any ci-pipeline.

Improvement Proposal 
For setting up a pipeline we need the following things (sorted by importance):

• Repo on the cac gitlab
• CI-Server on our cluster
• A name for our cluster (I would propose Entoron)
• A Makefile for our code
• A gitlab-yaml file with the configuration
• A code-coverage tool
• rules for pushing/merging PRs
• GoReport Card

The Travis CI is currently in a failure state. This should be fixed.

Furthermore, travis-ci.org has a banner that the service will shut down in a few weeks and accounts will be moved to travis-ci.com. This will probably cause us to reconfigure the Travis CI script. On this occasion, we could just switch to using GitHub Actions for CI. Or is there an argument against that?

There are several issues with the Makefile, probably related to the fact that this is a go module.

go list

go list github.com/perun-network/go-perun, which is called by the Makefile at the beginning, returns

can't load package: package github.com/perun-network/go-perun: unknown import path "github.com/perun-network/go-perun": cannot find module providing package github.com/perun-network/go-perun

make dep

AFAIK go get is not required any more with modules

make build

We don't have any binary yet to build. Probably the test target, which runs go test, is sufficient for the foreseeable future.