Description

We want to add tests to increase coverage to 80% and gain certainty over our code. Currently we are between 60-70%. Use your judgment regarding which tests to add.

Acceptance criteria

Please breakup PRs by package

Ordered in prioritiy:

• types
• core
• node
• abi
• commands
• mining
• wallet
• config
• repo

Risks + pitfalls

• See (inconclusive) discussion below on coverage vs. focus
• Avoid tests that lock in a specific implementation; test I/O instead (see contributing.md)

Where to begin

When we load, edit and store data from the blockchain as well as other related data we have similar patterns that can be seen on classical data base access.

For this reason we would like to explore building a activerecord like pattern for those datastructures.

• Disable Appveyor
• Circle CI happy
  • Lint
  • Test
  • Deps

I would like to propose to use something like https://github.com/go-playground/validator to allow simple validation of the content of structs, both on creation and on marshal/unmarshal.

Using the above package we could write something like this

type Block struct {
  RequiredField int `validation:"required"`
  OptionalField uint
}

func NewBlock(f1 int, f2 uint) (*Block, error) {
  out := &Block{
    RequiredField: f1,
    OptionalField: f2,
  }
  err := validate.Struct(out)
  if err != nil {
    return err
  }
  return out, nil
}

Ref: ipfs/kubo#3065

Currently a lot of dependencies are not cached at all on CI. Making it slower than it has to be.

Ref: libp2p/research-pubsub#17

• implement using https://github.com/goreleaser/goreleaser
• document
• create builds for all three major platforms (osx, linux windows)
• upload to github releases

In ethereum, if a contract invocation fails, all state is reverted, including any logs. This makes debugging things a nightmare.

We need to make sure that method calls, even ones that fail, should still be able to return some sort of information to the called.

We currently have most linters that gometalinter provides enabled.

But, to quote @whyrusleeping

Your gometalinter config is excessive, take a look at how ethereums works: https://github.com/ethereum/go-ethereum/blob/master/build/ci.go#L327

So lets decide on which ones we want to have in this issue and make the change.

List of linters currently enabled

• maligned
• deadcode
• dupl
• errcheck
• gas
• goconst
• gocyclo
• gofmt
• goimports
• golint
• gotype
• gotypex
• ineffassign
• interfacer
• megacheck
• structcheck
• unconvert
• varcheck
• vet
• vetshadow
• unparam

List of linters that ethereum runs with

• vet
• gofmt
• misspell
• goconst
• gosimple
• unconvert

Independently we should do something like ethereum does, and run fast linters first, and the slower ones afterwards to fast failures on those during development.

List of things this entails

• persist the libp2p identity on disk
• persist mined files on disk
• persist the blockchain on disk
• implement locking using https://github.com/ipfs/go-fs-lock

It contains a reference to data. I can see the deal as output of my command.

The comments on f6d09de and #31 make it clear we need to talk about testing strategy. Let's punt for the moment on the discussion of cucumber vs testify, which at this point is an easy switch to make in either direction (let's talk about it once we have the larger issue resolved).

Here's how I see it, let me know if you see it differently.

We have a server, which is the node and the command implementations in Go. We have a CLI that presently doesn't implement much logic, it just dispatches commands to the server and relays results. The server has a lot of state and behavior. There are many ways it could be set up / configured and often we want to tightly control its state and/or behavior for testing so we can assert something very specific about what it does. For example we often want to stub/fake out a service or data structure the server relies on for a test. For this reason comprehensive functional testing of server behavior should test the server directly, in Go. It should test the server directly (as opposed to through a proxy like a CLI) because that's the easy way to be able to have fine-grain control over its state and behavior: by programmatically building a server instance appropriate for whatever test you want to write. This should happen in Go not only because that's the easy way to build a server instance for testing but also for the reason we use Go in the first place: it's powerful and expressive and there are lots of tools and libraries to work with.

The CLI serves two primary purposes. It can (doesn't yet, but probably will) implement sophisticated logic (eg, for making deals and sending data). This sophisticated logic should be tested directly, in Go (and not through a proxy like a CLI) for the same reasons above. The second purpose of the CLI is to relay commands to the server. Since any sophisticated logic is tested directly in Go for both pieces, mostly what we care about is an end-to-end test that verifies that the pieces are hooked up properly and can talk to one another. Beyond "can we send command X and get back what looks like a reasonable response", we shouldn't comprehensively test server functionality from behind the CLI (for the reasons above).

We could write the end to end tests for the CLI in Go or in another language like shell. Personally I have a strong preference for Go -- if we do shell the context switch is hard, it's a terrible programming language, there aren't good packages to build on, etc etc etc. However if we have something tried and true that we can just drop in and if we are not trying to do functional testing of the server beyond verifying e2e they can talk to each other and verifying some of the shelly glue (does it start and stop, etc), I'm OK with it. What I think would be sub-optimal for our ability to write nice tests would be to try to comprehensively functional test the server from on the other side of the CLI.

I do think we need an additional level of testing beyond direct unit functional testing in Go and e2e testing of the CLI. I think we want what we'd call integration tests where we spin up a bunch of nodes and make assertions about them. I understand IPFS has something like this. I'd like to cross that bridge when we get to it, or maybe in this convo after present discussion and the cucumber vs testify discussion.

So @dignifiedquire @whyrusleeping: any major points of departure from the above?

Also I'd like to take a crack at server testing as part of the story I'm doing, I've done quite a bit of this and there are some common patterns we can try.

The spec is the ultimate commentary on the code, maybe it should live alongside/in the code? We should consider building a tool that generates the spec automatically from repo files, comments & code. Like godoc or javasdoc, but for the spec.

Spitballing here, hear me out. Looking over what's in the spec we have a few different things.

• General commentary, motivation, and whatnot: this could live in a markdown file we use as the skeleton of the spec document. Additional content could be extracted from the source files into this skeleton.
• Data structures: it's easy to annotate structures and fields that are top-level concepts either in comments // @TopLeveLConcept StateTree or using actual `pl.spec` annotations on fields, and we could use annotations that map to spec types if we wanted. Labeled comment sections and these data structures could be extracted directly.
• Algorithms: should be in comments anyway. When we extract this description it's easy to automatically back-link to the code.
• APIs are easy to annotate and extract. If we had an IDL that generated Go it'd be even easier, but we don't need it.
• Other kinds of high-level descriptions like security properties: could be drawn from comments, readme/markdown files that live alongside the code

We could include composition hints in the comments or code for how to merge pieces from different source files (/at/Section Foo).

Advantages: reduces risk of diversion between spec and implementation as we implement. You can see what is and is not implemented at a glace (annotations could differentiate them). We could flag changes that affect spec in code reviews.

When we are "done" implementing we could detach the output spec from its inputs and just use it as a stand alone file if we wanted.

• uses github releases
• configuration option to turn off and on, default is on
• configuration on how often to check for a new release, default to, on startup + once per day
• runs a main process, which will react to a new release, download it, verify and if correct replace the currently running binary with the new one
• possible libraries
  • https://github.com/inconshreveable/go-update
  • https://github.com/jpillora/overseer

Currently one has to manually specify ports, and be careful of calling out to which daemon. This needs to be improved to avoid accidental errors in this area.

I am not sure on the best api for this yet, suggestions are very welcome.

Requirements

• a cli command: go-filecoin client get <DealID>
• a method on the storage market actor to retrieve the details about a deal from the deal id
• a way to given the full details of a deal to retrieve the data from the miner

Dependencies

• #27
• #26
• #25

Doesn't say anything about transitivity.

Description

If a user configures a filecoin node and restarts, the configurations should persist in the restarted node. See #109 (WIP) for context on nodes, and #45 for details on configuration files.

Acceptance criteria

• the configuration file is the source of truth for all settings
• there will be helper commands later on to change it
• use toml as configuration format for now
• if there is no config file provided, print a warning and assume the default configuration
• default location is the home directory .filecoin/config
• there should be a command line flag to specify the location for the file to look in
• there should be a command to reload the config file from disk to apply changes

Risks + pitfalls

Where to begin

• Look at the options for commands.

There is no validation (or rather, it always returns ok).

Use the command library.

Description

We need types for these 2:

• storage (something like Bytes)
• filecoin amounts (something like TokenAmount)

Currently, they are mostly represented by BigInts which is way too permissive. An added benefit of switching to custom types will be that function signatures will show the correct type and not a generic big.Int.

Acceptance Criteria

• Interacting with these types is done through wrappers. The wrappers provide safer methods, but we still store the actual values.

• Make it easy to read comparisons, etc.

Risks + Pitfalls

N/A

Where to begin

• Example of both types being used: https://github.com/filecoin-project/go-filecoin/blob/master/core/miner_actor.go#L24-L30

A lot of thinking and careful design has gone into the fundamental filecoin interfaces. It's important that we capture that intent so that we don't accidentally violate security properties or important design elements (eg, that will be important to build upon down the road). Juan suggested we write the fundamental interfaces in Go and capture in comments their rationale and requirements. The interfaces we write aren't set in stone, but deviations should be carefully considered and we need to preserve the rationale and requirements.

ref: ipfs/kubo#4213

I am used to squashing PRs, to get a cleaner git history with better descriptions, which I did for #43. But going forward we should agree on one strategy.

I would suggest one of these two

1. squash commits into a single commit for each PR, by the person merging, or
2. after approval of the PR the creator of the PR squashes the commits to one or more commits with clean commit messages, before it gets merged

Both these options would allow us to create & maintain a well readable git history, while still being able to iterate fast and have small commits with so and so commit messages when working on something.

Blocked on #33

Description

Currently we use mostly naive go channels for this, but we need to ensure all callers and receivers are in the correct order, no matter what.

Acceptance Criteria

1. the 'wait for message' code wants to be able to see every message we 'accept'. In this case, we will want to make sure we send every block in order that it appears on the chain (even if we accept a block two ahead, or theres a reorg)

2. The miner code, that always wants the latest block to mine off of. In this case we really want a slightly different thing than case 1, as we likely don't care about intermediate blocks, we just want to know what to mine on.

Risks + Pitfalls

Where to begin

• https://github.com/filecoin-project/go-filecoin/blob/master/mining/worker.go
• https://github.com/filecoin-project/go-filecoin/blob/master/core/chain_manager.go#L79
• https://github.com/filecoin-project/go-filecoin/blob/master/node/node.go#L238
• https://github.com/filecoin-project/go-filecoin/blob/master/commands/command_utils.go#L30