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

Consumer issues new promise for the provider #362

Merged
merged 19 commits into from Oct 8, 2018

Conversation

Projects
None yet
5 participants
@soffokl
Copy link
Member

commented Sep 13, 2018

This PR includes "Promise balance notifications": #381

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

@soffokl soffokl self-assigned this Sep 13, 2018

@soffokl soffokl requested review from tadovas and Waldz as code owners Sep 13, 2018

@tadovas
Copy link
Member

left a comment

As it's still WIP, a simple comment - it feels like connection manager does way too much and knows too much. Why cannot session handler take care of promise issueing, checking or some new dedicated component. The main reason of connection manager - to keep dialog and underlying service states in sync. I.e. close dialog when service disconnects

@@ -45,6 +47,9 @@ func NewCommand() *cli.Command {
return di.Node.Wait()
},
After: func(ctx *cli.Context) error {
if di.Node == nil {
return fmt.Errorf("Node instance does not exist")

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 14, 2018

Member

You dont need to thow error, just:

if di.Node != nil {
    return di.Node.Kill()
}
return nil
@@ -33,6 +35,10 @@ type DialogCreator func(consumerID, providerID identity.Identity, contact dto.Co
// consumer identity, provider identity and uses state callback to report state changes
type VpnClientCreator func(session.SessionDto, identity.Identity, identity.Identity, state.Callback, ConnectOptions) (openvpn.Process, error)

// PromiseCreator creates a new promise from consumer to provider.
// Consumer issues and signs this promise.
type PromiseCreator func(consumerID, providerID identity.Identity, amount money.Money) (*promise.SignedPromise, error)

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 14, 2018

Member
  • consumerID -> IssuerID
  • providerID -> BenefiterID
    because this process does not have to be done by consumers-only

response := responsePtr.(*Response)
if err != nil || !response.Success {
return nil, errors.New("PromiseDto create failed. " + response.Message)

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 14, 2018

Member

PromiseDto create failed-> Promise issuing failed

Show resolved Hide resolved core/connection/manager_test.go Outdated

const endpoint = "promise-create"

func NewPromise(consumerID, providerID identity.Identity, amount money.Money) *Promise {

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 14, 2018

Member
  • consumerID -> IssuerID
  • providerID -> BenefiterID
}
}

func NewSignedPromise(promise *Promise, signer identity.Signer) (*SignedPromise, error) {

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 14, 2018

Member

NewSignedPromise -> SignByIssuer()
signer -> issuerSigner

Amount: amount,
}

assert.Equal(t, 1, promise.SerialNumber)

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 14, 2018

Member

Does this test just struct constructor. Do we need such?


type handler struct{}

func NewDialogHandler() *handler {

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 14, 2018

Member

promiseChecker?

@soffokl soffokl force-pushed the feature/issue-promise branch 2 times, most recently from 4638307 to 01f735a Sep 20, 2018

@soffokl soffokl changed the title WIP Consumer issues new promise for the provider Consumer issues new promise for the provider Sep 21, 2018

@soffokl soffokl requested review from zolia and vkuznecovas Sep 21, 2018

@soffokl

This comment has been minimized.

Copy link
Member Author

commented Sep 21, 2018

@tadovas @vkuznecovas @Waldz @zolia
the basic promise issuing ready for review

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


// Request structure represents message from service provider to receive new promise from consumer
type Request struct {
SignedPromise *SignedPromise

This comment has been minimized.

Copy link
@zolia

zolia Sep 26, 2018

Member

Why to have SignedPromise inside Request? Its just 1 member, why not simply SignedPromise?

This comment has been minimized.

Copy link
@soffokl

soffokl Oct 3, 2018

Author Member

I think keeping request/response here, how it used in the communication dialog makes it more clear for understanding.

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 5, 2018

Member

Keep in mind, that will send SignedPromise structure to blockhain for clearing reasons

}
promiseAmountFlag = cli.IntFlag{
Name: "promise.amount",
Usage: "Amount of money that will be used for issuing a single promise",

This comment has been minimized.

Copy link
@zolia

zolia Sep 26, 2018

Member

We should specify format for a user. I.e: Integer, 1000 == 1 MYST

This comment has been minimized.

Copy link
@soffokl

soffokl Oct 3, 2018

Author Member

I was thinking that it will be minimal allowed value, as a wei for Ether.

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 3, 2018

Member

Do we need configurable promise at all at this stage.
We agreed to issue promise which has full amount from serviceProposal

type MessageProducer interface {
// GetMessageEndpoint returns endpoint there to send messages

This comment has been minimized.

Copy link
@zolia

zolia Sep 26, 2018

Member

"GetMessageEndpoint returns an endpoint where to send messages"

Produce() (messagePtr interface{})
}

// MessageConsumer represents instance which handles messages of specific endpoint
type MessageConsumer interface {
// GetMessageEndpoint returns endpoint there to receive messages

This comment has been minimized.

Copy link
@zolia

zolia Sep 26, 2018

Member

"GetMessageEndpoint returns an endpoint where to receive messages"

// NewMessage creates struct where message from endpoint will be serialized
func (consumer *BalanceMessageConsumer) NewMessage() (messagePtr interface{}) {
var message BalanceMessage
return &message

This comment has been minimized.

Copy link
@zolia

zolia Sep 26, 2018

Member

we could just write return &BalanceMessage{}


// BalanceMessageConsumer process balance notification from communication channel.
type BalanceMessageConsumer struct {
Callback func(*BalanceMessage)

This comment has been minimized.

Copy link
@zolia

zolia Sep 26, 2018

Member

Can we name it ConsumeCallback ?

// Consume handles messages from endpoint
func (consumer *BalanceMessageConsumer) Consume(messagePtr interface{}) error {
consumer.Callback(messagePtr.(*BalanceMessage))
return nil

This comment has been minimized.

Copy link
@zolia

zolia Sep 26, 2018

Member

wired to have error contract, but no errors produced..

type RequestEndpoint string

// RequestProducer represents instance which creates requests/responses of specific endpoint
type RequestProducer interface {
// GetRequestEndpoint returns endpoint there to send requests

This comment has been minimized.

Copy link
@vkuznecovas

vkuznecovas Sep 26, 2018

Contributor

Typo: "there" -> "where"

}

// RequestConsumer represents instance which handles requests/responses of specific endpoint
type RequestConsumer interface {
// GetRequestEndpoint returns endpoint there to receive requests

This comment has been minimized.

Copy link
@vkuznecovas

vkuznecovas Sep 26, 2018

Contributor

Typo: "there" -> "where"


const issuerLogPrefix = "[promise-issuer] "

// PromiseIssuer issues promises in such way, what no actual money is added to promise

This comment has been minimized.

Copy link
@vkuznecovas

vkuznecovas Sep 26, 2018

Contributor

Typo: "what" -> "that"

@@ -0,0 +1,44 @@
/*

This comment has been minimized.

Copy link
@vkuznecovas

vkuznecovas Sep 26, 2018

Contributor

bad naming of the file, should be balance_consumer.go

// Consumer needs to know how Provider sees a promise:
// - does Provider still accept previous promise
// - amount left in promise
// - speed how Provider deducts money from promise

This comment has been minimized.

Copy link
@vkuznecovas

vkuznecovas Sep 26, 2018

Contributor

where's the speed?

)

func TestBalanceMessageConsumer_Interface(t *testing.T) {
var _ communication.MessageConsumer = &BalanceMessageConsumer{}

This comment has been minimized.

Copy link
@vkuznecovas

vkuznecovas Sep 26, 2018

Contributor

These test files seem odd. At the moment there are no tests, and the interface implementation check could be added just to the non-test version of the file. Why is there a need for such check anyway? If you fail to implement it, that will be caught at build time, right?

BenefiterID string
Amount money.Money
func TestBalanceMessageProducer_Interface(t *testing.T) {
var _ communication.MessageProducer = &BalanceMessageProducer{}

This comment has been minimized.

Copy link
@vkuznecovas

vkuznecovas Sep 26, 2018

Contributor

These test files seem odd. At the moment there are no tests, and the interface implementation check could be added just to the non-test version of the file. Why is there a need for such check anyway? If you fail to implement it, that will be caught at build time, right?

)

func TestPromiseIssuer_Interface(t *testing.T) {
var _ connection.PromiseIssuer = &PromiseIssuer{}

This comment has been minimized.

Copy link
@vkuznecovas

vkuznecovas Sep 26, 2018

Contributor

These test files seem odd. The interface implementation check could be added just to the non-test version of the file. Why is there a need for such check anyway? If you fail to implement it, that will be caught at build time, right?

)

func TestPromiseProcessor_Interface(t *testing.T) {
var _ session.PromiseProcessor = &PromiseProcessor{}

This comment has been minimized.

Copy link
@vkuznecovas

vkuznecovas Sep 26, 2018

Contributor

These test files seem odd. At the moment there are no tests, and the interface implementation check could be added just to the non-test version of the file. Why is there a need for such check anyway? If you fail to implement it, that will be caught at build time, right?

}

func Test_SignedPromise(t *testing.T) {

This comment has been minimized.

Copy link
@vkuznecovas

vkuznecovas Sep 26, 2018

Contributor

You don't seem to be testing any logic here, just asserting that Go assigns values correctly

Waldz and others added some commits Oct 3, 2018

soffokl added some commits Sep 13, 2018

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

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

case *session.CreateProducer:
return &session.CreateResponse{
Success: true,
Message: "Everything is great!",

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 3, 2018

Member

This field is used for errors only Success: false

This comment has been minimized.

Copy link
@soffokl

soffokl Oct 5, 2018

Author Member

Removed Message.

promiseIssuerFactory := func(dialog communication.Dialog) connection.PromiseIssuer {
return noop.NewPromiseIssuer(dialog)
promiseIssuerFactory := func(issuerID identity.Identity, dialog communication.Dialog) connection.PromiseIssuer {
return &noop.PromiseIssuer{

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 3, 2018

Member

Use factory here

This comment has been minimized.

Copy link
@soffokl

soffokl Oct 5, 2018

Author Member

Changed it to use NewPromiseIssuer.

IssuerID: issuerID,
Dialog: dialog,
Signer: signerFactory(issuerID),
Amount: money.Money{

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 3, 2018

Member

You are getting issuer.Start(proposal) and that data should be enough to create promises, because it holds primise.PaymentMethod.
Probably we need something as NoopPaymentMethod

Later:
We agreed to create PromisePolicy interface , which answers to political questions. We can implement it with later PR.

This comment has been minimized.

Copy link
@soffokl

soffokl Oct 5, 2018

Author Member

Changed it to getting Amount from the promise

Show resolved Hide resolved core/promise/consumer.go
type PromiseIssuer struct {
dialog communication.Dialog
IssuerID identity.Identity
Dialog communication.Dialog

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 3, 2018

Member

Private attributes and factories. Caller will now know how to initiate all runtime variables

This comment has been minimized.

Copy link
@soffokl

soffokl Oct 5, 2018

Author Member

Changed it to be a private.

return &CreateRequest{
ProposalId: producer.ProposalID,
}
}

// RequestSessionCreate requests session creation and returns session DTO
func RequestSessionCreate(sender communication.Sender, proposalID int, sessionPtr *Session) error {
responsePtr, err := sender.Request(&createProducer{

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 3, 2018

Member

Why to make public? Why to refactor session at all?

This comment has been minimized.

Copy link
@soffokl

soffokl Oct 5, 2018

Author Member

Changed it back to be unexported.

}

// Send sends signed promise via the communication channel
func Send(signedPromise *SignedPromise, sender communication.Sender) (*Response, error) {

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 3, 2018

Member

Does it belong to DTO file?
Maybe should be similar approach as RequestSessionCreate- helper on provider side

This comment has been minimized.

Copy link
@soffokl

soffokl Oct 5, 2018

Author Member

I think Send is related only to SignedPromise, I have changed it to be a method of it to point it explicitly. It was added to the promise.go file since it related only to promises.

return nil, errors.New("Promise issuing failed: " + response.Message)
}

return response, nil

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 3, 2018

Member

Do we need response that promise was received? What data will be there?
Very hard to see the case without Send being called in some actual place

This comment has been minimized.

Copy link
@soffokl

soffokl Oct 5, 2018

Author Member

Removed response.

@soffokl soffokl force-pushed the feature/issue-promise branch from d2b5511 to b6974c1 Oct 4, 2018

@soffokl soffokl requested review from Waldz, vkuznecovas and zolia Oct 4, 2018

@soffokl soffokl force-pushed the feature/issue-promise branch from 2f172bb to 023c41e Oct 4, 2018

@Waldz

Waldz approved these changes Oct 5, 2018

@soffokl soffokl merged commit dd67306 into master Oct 8, 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

@soffokl soffokl deleted the feature/issue-promise branch Oct 9, 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.