vegaprotocol / vega

A Go implementation of the Vega Protocol, a protocol for creating and trading derivatives on a fully decentralised network.
https://vega.xyz
GNU Affero General Public License v3.0
37 stars 22 forks source link

The stats type is exposed directly in the api package #1534

Open EVODelavega opened 4 years ago

EVODelavega commented 4 years ago

in trading_data.go, in the API package, we're getting values from the stats type for the API. This is fine, but the stats.Stats type is passed in raw, so the Inc*, Add* and Set* methods are all exposed. We ought to define an interface so only getters are exposed to the api package.

TODO:

Tests:

The stats and monitoring objects are the only dependencies not yet changed to an interface. Once we have the stats dependency as an interface, we can easily write a unit test to cover the statistics endpoint.

Risks:

As previously indicated, name conflicts are possible, and resolving these incorrectly could result in bugs. For this reason, I'd recommend using an overarching getter unless we can be absolutely certain that renaming the getter will work as expected. Just an example to clarify what I mean:

type Stats struct {
    *Blockchain // as opposed to the current Blockchain *Blockchain
    *FutureStats // currently no other stats objects exist/are embedded
}

func (b Blockchain) GetFoo() int {
    return b.foo
}

func (f FutureStats) GetFoo() int {
    return f.foo
}

func (s Stats) GetBlockchainFoo() int {
    return s.Blockchain.GetFoo()
}

func (s Stats) GetFutureFoo() int {
    return s.foo.GetFoo()
}

// and not:
func (f FutureStat) GetFutureFoo() int {
    return f.foo
}

// with expected use of:
stat.GetFoo() // equivalent to stat.Blockchain.GetFoo()
stat.GetFutureFoo() // equivalent to stat.FutureStat.GetFoo()
edd commented 4 years ago

@EVODelavega Let us know if this is done

gordsport commented 3 years ago

@EVODelavega - please can you confirm if this is still required or is done?