Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added promises validation and storing for the service provider. #347 #404

Merged
merged 9 commits into from Oct 17, 2018

Conversation

Projects
None yet
6 participants
@soffokl
Copy link
Member

commented Sep 25, 2018

Closes #347

@soffokl soffokl added this to the Working promise engine POC milestone Sep 25, 2018

@soffokl soffokl self-assigned this Sep 25, 2018

@soffokl soffokl requested review from Waldz, zolia and vkuznecovas Sep 25, 2018

@soffokl soffokl requested a review from tadovas as a code owner Sep 25, 2018

@soffokl soffokl force-pushed the store-promises branch from 2fdbba7 to 52a8a63 Sep 25, 2018

)

var (
errLowAmount = fmt.Errorf("promise amount less then the service proposal price")

This comment has been minimized.

Copy link
@vkuznecovas

vkuznecovas Sep 25, 2018

Contributor

typo "then" -> "than"


var (
errLowAmount = fmt.Errorf("promise amount less then the service proposal price")
errLowBalance = fmt.Errorf("issuer balance less then the promise amount")

This comment has been minimized.

Copy link
@vkuznecovas

vkuznecovas Sep 25, 2018

Contributor

typo "then" -> "than"


signature := identity.SignatureBase64(string(request.SignedPromise.IssuerSignature))
issuer := identity.FromAddress(request.SignedPromise.Promise.IssuerID)
verify := identity.NewVerifierIdentity(issuer)

This comment has been minimized.

Copy link
@vkuznecovas

vkuznecovas Sep 25, 2018

Contributor

this could be better named as verifier, since the line below "verify.Verify" does seem a bit confusing

balance, err := c.etherClient.BalanceAt(context.Background(), common.HexToAddress(issuer.Address), nil)
if err != nil {
return nil, err
} else if balance.Uint64() < amount {

This comment has been minimized.

Copy link
@vkuznecovas

vkuznecovas Sep 25, 2018

Contributor

redundant else

if err != nil {
return nil, err
} else if balance.Uint64() < amount {
return &Response{

This comment has been minimized.

Copy link
@vkuznecovas

vkuznecovas Sep 25, 2018

Contributor

this forming of responses seems to be a bit repetitive, maybe create a helper function like "ResponseFromError(err error, request)"

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 5, 2018

Member

I suggest static response with const responseInsufficient = ..

@soffokl

This comment has been minimized.

Copy link
Member Author

commented Sep 25, 2018

@vkuznecovas comments are fixed.

@soffokl soffokl requested a review from vkuznecovas Sep 26, 2018

)

type consumer struct{}
var (
errLowAmount = fmt.Errorf("promise amount less than the service proposal price")

This comment has been minimized.

Copy link
@zolia

zolia Sep 26, 2018

Member

Why not errors.New()? You dont need formatting in this case.

This comment has been minimized.

Copy link
@soffokl

soffokl Sep 26, 2018

Author Member

Well, both errors.New() and fmt.Errorf() doing the same under the hood.
It's more style question. Do we have any recommendation which one to use in the project?

This comment has been minimized.

Copy link
@zolia

zolia Sep 26, 2018

Member

Most of the time we used errors.New(), sorta indicates that we are creating "New" message.

This comment has been minimized.

Copy link
@soffokl

soffokl Sep 26, 2018

Author Member

ok, clear, changed it.

@soffokl soffokl force-pushed the feature/issue-promise branch 3 times, most recently from 7a78b88 to d27f7bd Oct 3, 2018

@soffokl soffokl force-pushed the store-promises branch 2 times, most recently from 0cfff1f to f28bf33 Oct 4, 2018

cmd/di.go Outdated
) session.ManagerFactory {
return func(dialog communication.Dialog) session.Manager {
promiseProcessor := noop.NewPromiseProcessor(dialog)
promiseProcessor := noop.NewPromiseProcessor(dialog, etherclient, promiseStorage)

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 5, 2018

Member

Lets inject promiseProcessor directly, instead of it's dependencies

This comment has been minimized.

Copy link
@soffokl

soffokl Oct 10, 2018

Author Member

Moved dependencies to the external factory.

@@ -113,7 +116,7 @@ func NewCommand(licenseCommandName string) *cli.Command {
}()

go func() {
if err := di.ServiceManager.Start(); err != nil {
if err := di.ServiceManager.Start(di.EtherClient); err != nil {

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 5, 2018

Member

Dont inject on runtime, lets inject in factory

This comment has been minimized.

Copy link
@soffokl

soffokl Oct 10, 2018

Author Member

Fixed.

cmd/di.go Outdated
@@ -113,7 +115,7 @@ func (di *Dependencies) bootstrapNodeComponents(nodeOptions node.Options) {
}

// BootstrapServiceComponents initiates ServiceManager dependency
func (di *Dependencies) BootstrapServiceComponents(nodeOptions node.Options, serviceOptions service.Options) {
func (di *Dependencies) BootstrapServiceComponents(nodeOptions node.Options, serviceOptions service.Options) error {

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 5, 2018

Member

I would bootstrap storage/bolt instance on Bootstrap(), because it will be needed for several dependencies.
In this why You will skip error here.

This comment has been minimized.

Copy link
@soffokl

soffokl Oct 10, 2018

Author Member

Moved storage to the di.

cmd/di.go Outdated
@@ -125,6 +127,10 @@ func (di *Dependencies) BootstrapServiceComponents(nodeOptions node.Options, ser
discoveryService := discovery.NewService(di.IdentityRegistry, di.IdentityRegistration, di.MysteriumClient, di.SignerFactory)

sessionStorage := session.NewStorageMemory()
promiseStorage, err := storage.NewStorage(nodeOptions.Directories.Data)

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 5, 2018

Member

Can we have intermediate variable OptionsDirectory.Storage, like it is with OptionsDirectory.Keystore

This comment has been minimized.

Copy link
@soffokl

soffokl Oct 10, 2018

Author Member

Added.

import (
"path/filepath"

"github.com/mysteriumnetwork/node/core/promise/storage/boltdb"

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 5, 2018

Member

This package promise/storage should be decoupled from boltdb, and whould depend only on interface Storage

This comment has been minimized.

Copy link
@soffokl

soffokl Oct 10, 2018

Author Member

Changed it.

return nil, err
}

return &Response{Success: true, Message: "Promise accepted", Request: request}, nil

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 5, 2018

Member

We dont need to resend request back

This comment has been minimized.

Copy link
@soffokl

soffokl Oct 10, 2018

Author Member

Removed it. It was added to simplify matching error responses to the requests for debugging.

}

func (fb *fakeBlockchain) BalanceAt(ctx context.Context, account common.Address, blockNumber *big.Int) (*big.Int, error) {

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 5, 2018

Member

If would have smaller interface, You would not need to mock all other functions StorageAt, NonceAt

This comment has been minimized.

Copy link
@soffokl

soffokl Oct 10, 2018

Author Member

Fixed.

@@ -23,8 +23,10 @@ import (
"time"

log "github.com/cihub/seelog"
ethereum "github.com/ethereum/go-ethereum"

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 5, 2018

Member

Lets decouple PromiseProcessor from Ethereum

This comment has been minimized.

Copy link
@soffokl

soffokl Oct 10, 2018

Author Member

Done.

@@ -80,6 +87,7 @@ func (processor *PromiseProcessor) Start(proposal discovery_dto.ServiceProposal)

// Stop stops processing promises
func (processor *PromiseProcessor) Stop() error {
processor.storage.Close()

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 5, 2018

Member

Can we stop storage it Node.Stop because this dependency will be needed in more places

This comment has been minimized.

Copy link
@soffokl

soffokl Oct 10, 2018

Author Member

Added storage stopping to the di.Shutdown()

Show resolved Hide resolved e2e/connection_test.go
if err != nil {
return nil, err
}
if balance.Uint64() < amount {

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 5, 2018

Member

Can BalanceRegistry return already parsed money?

This comment has been minimized.

Copy link
@soffokl

soffokl Oct 10, 2018

Author Member

For money.Money we need to know Currency to provide it. Left only balance, since we don't know currency at that moment.

@soffokl soffokl changed the base branch from feature/issue-promise to master Oct 9, 2018

soffokl added some commits Oct 4, 2018

@soffokl soffokl force-pushed the store-promises branch from f28bf33 to f5a16ce Oct 10, 2018

@soffokl soffokl requested review from Waldz and vkuznecovas Oct 10, 2018

// TODO there should be some validation of the received proposal and storing it somewhere for the server needs.
// TODO signature validation of the promise should be here too.
if err := request.SignedPromise.Validate(c.proposal, c.balanceRegistry); err != nil {
return failedResponse(err), nil

This comment has been minimized.

Copy link
@zolia

zolia Oct 10, 2018

Member

prob here we should return err also: return failedResponse(err), err

This comment has been minimized.

Copy link
@soffokl

soffokl Oct 11, 2018

Author Member

Changed.

// }, fmt.Errorf("Bad promise signature")
// }
if err := c.storage.Store(request.SignedPromise.Promise.IssuerID, &request.SignedPromise.Promise); err != nil {
return nil, err

This comment has been minimized.

Copy link
@zolia

zolia Oct 10, 2018

Member

err not returned too. We should be able to log all improper activity by client on service log...

This comment has been minimized.

Copy link
@soffokl

soffokl Oct 11, 2018

Author Member

Fixed.

SerialNumber int `storm:"id"`
IssuerID string `storm:"index"`
BenefiterID string `storm:"index"`
Fee money.Money

This comment has been minimized.

Copy link
@zolia

zolia Oct 10, 2018

Member

I would stick with 'Amount'. Promise usually is not actual 'Fee', but just promised 'Amount'.

This comment has been minimized.

Copy link
@soffokl

soffokl Oct 11, 2018

Author Member

Amount is back.

processor.lastPromise = promise.Promise{
Amount: money.NewMoney(10, money.CURRENCY_MYST),
Fee: money.NewMoney(10, money.CURRENCY_MYST),

This comment has been minimized.

Copy link
@zolia

zolia Oct 10, 2018

Member

Why '10'? We prob should externalise this one.

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 10, 2018

Member

I think will be improved in upcoming PRs. TODO needed

This comment has been minimized.

Copy link
@soffokl

soffokl Oct 11, 2018

Author Member

Added TODO here too.

Show resolved Hide resolved cmd/di.go
@@ -95,7 +102,7 @@ balanceLoop:

case <-time.After(processor.balanceInterval):
processor.balanceSend(
promise.BalanceMessage{1, true, processor.lastPromise.Amount},
promise.BalanceMessage{1, true, processor.lastPromise.Fee},

This comment has been minimized.

Copy link
@zolia

zolia Oct 10, 2018

Member

We just could have TODO here, since no actual requestID present.

This comment has been minimized.

Copy link
@soffokl

soffokl Oct 11, 2018

Author Member

Added TODO

)

// BalanceRegistry provides amount of money that identity have on the balance in the blockchain
type BalanceRegistry func(Identity) (uint64, error)

This comment has been minimized.

Copy link
@zolia

zolia Oct 10, 2018

Member

Since it is just 1 function, I would name it with verb, like BalanceCheck, it is always better readable to see some action if you getting some data.

This comment has been minimized.

Copy link
@soffokl

soffokl Oct 11, 2018

Author Member

Changed it to Balance since "Effective go" recomends such approach for getters.

@zolia
Copy link
Member

left a comment

minor issues

@soffokl

This comment has been minimized.

Copy link
Member Author

commented Oct 11, 2018

@Waldz @zolia It's ready for review.

@donce donce referenced this pull request Oct 11, 2018

Closed

Sessions endpoint #433


// Storage stores persistent objects for future usage
type Storage interface {
Store(issuer string, data interface{}) error

This comment has been minimized.

Copy link
@tadovas

tadovas Oct 16, 2018

Member

Issuer has nice datat type which is called Identity. Can we reuse it here? string type is too generic

This comment has been minimized.

Copy link
@soffokl

soffokl Oct 17, 2018

Author Member

It receives string to not depend on any internal things.

// Validate check signed promise to be valid. It checks signature, benefiter address.
// Also it compares the promised amount to be enough for the proposal.
// And finally it checks that issuer have enough balance to issue the promice.
func (sp *SignedPromise) Validate(proposal dto.ServiceProposal, balance identity.Balance) error {

This comment has been minimized.

Copy link
@tadovas

tadovas Oct 16, 2018

Member

It feels like this validate does too much. Why it has to depend on proposal? Signed promise with valid signature itself is perfectly valid, as both parties agreed on amount and other parameters

This comment has been minimized.

Copy link
@soffokl

soffokl Oct 17, 2018

Author Member

Requirements for validation was provided in the issue: #347

SerialNumber int
IssuerID string
BenefiterID string
SerialNumber int `storm:"id"`

This comment has been minimized.

Copy link
@tadovas

tadovas Oct 16, 2018

Member

I really really recommend to start using Promise structures from payments lib, as these promises will land on contract anyway, and a lot of work will need to be done to align them with payment contract expected structure later

This comment has been minimized.

Copy link
@soffokl

soffokl Oct 17, 2018

Author Member

Looks like it's out of the scope of this PR.
Created issue for changing it in the same milestone. #454

@tadovas
Copy link
Member

left a comment

There are some key issues, which I would like to discuss more

@Waldz

Waldz approved these changes Oct 16, 2018

db *storm.DB
}

// NewStorage creates a new BoltDB storage for service promises

This comment has been minimized.

Copy link
@interro

interro Oct 16, 2018

Contributor

storage can be used for generic objects, for example for storing sessions too.
Functions comments can be replaced like this:
"for service promises" -> "for storing values"

This comment has been minimized.

Copy link
@soffokl

soffokl Oct 17, 2018

Author Member

For now, it's used only for storing promises, what is the main aim of the PR.
Let's replace the comment (and probably not comment only) when we will have something different.

Comments fixed

@Waldz Waldz merged commit ef25732 into master Oct 17, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@Waldz Waldz deleted the store-promises branch Oct 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.