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

Promise balance notifications #381

Merged

Conversation

Projects
None yet
3 participants
@Waldz
Copy link
Member

commented Sep 18, 2018

Closes #346

@Waldz Waldz requested a review from tadovas as a code owner Sep 18, 2018

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

@Waldz Waldz force-pushed the Waldz:feature/promise-balance-notifications branch from c728b39 to 94a062d Sep 18, 2018

@Waldz Waldz changed the title Promise balance notifications WIP Promise balance notifications Sep 18, 2018

@Waldz Waldz force-pushed the Waldz:feature/promise-balance-notifications branch 3 times, most recently from c0fe7be to a189215 Sep 18, 2018

)

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

This comment has been minimized.

Copy link
@soffokl

soffokl Sep 19, 2018

Member

We can move it to the global declaration, no func required here since this check will be done on compilation time.

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 26, 2018

Author Member

But this is pattern we are having already in 12 tests.
Is it wrong that test shows why this statements is there.

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 26, 2018

Author Member

Removed test function

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

This comment has been minimized.

Copy link
@soffokl

soffokl Sep 19, 2018

Member

The same, we can move it to the global declaration.

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 26, 2018

Author Member

Removed test function

}

func (issuer *PromiseIssuer) subscribePromiseBalance() error {
subscribeError := issuer.Dialog.Receive(

This comment has been minimized.

Copy link
@soffokl

soffokl Sep 19, 2018

Member

Just return issuer.Dialog.Receive(... is enough here.

)

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

This comment has been minimized.

Copy link
@soffokl

soffokl Sep 19, 2018

Member

The same, we can move it to the global declaration.

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 26, 2018

Author Member

Removed test function

func (generator *GeneratorFake) Generate() SessionID {
return generator.SessionIdMock
func TestPromiseProcessor_Interface(t *testing.T) {
var _ session.PromiseProcessor = &PromiseProcessor{}

This comment has been minimized.

Copy link
@soffokl

soffokl Sep 19, 2018

Member

The same, we can move it to the global declaration.

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 26, 2018

Author Member

Removed test function

type SaveCallback func(Session) error

// PromiseProcessor processes promises at provider side.
// Provider checks promises from consumer ans signs them also.

This comment has been minimized.

Copy link
@soffokl

soffokl Sep 19, 2018

Member

ans -> and

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 26, 2018

Author Member

fixed

@Waldz Waldz force-pushed the Waldz:feature/promise-balance-notifications branch from a189215 to 61451e2 Sep 19, 2018

@Waldz Waldz force-pushed the Waldz:feature/promise-balance-notifications branch 4 times, most recently from b96d1d0 to 5e9102e Sep 26, 2018

@Waldz Waldz changed the title WIP Promise balance notifications Promise balance notifications Oct 2, 2018

@Waldz Waldz removed the work in progress label Oct 2, 2018

@Waldz Waldz force-pushed the Waldz:feature/promise-balance-notifications branch from 5e9102e to 91938f9 Oct 2, 2018

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

@Waldz Waldz force-pushed the Waldz:feature/promise-balance-notifications branch 2 times, most recently from ea03bce to 040ee6c Oct 2, 2018

@soffokl soffokl force-pushed the Waldz:feature/promise-balance-notifications branch from a87f341 to 91938f9 Oct 2, 2018

@Waldz Waldz force-pushed the Waldz:feature/promise-balance-notifications branch 2 times, most recently from 82dafcf to 040ee6c Oct 2, 2018

@@ -61,15 +62,16 @@ type connectionManager struct {
}

// NewManager creates connection manager with given dependencies
func NewManager(mysteriumClient server.Client, dialogCreator DialogCreator,
func NewManager(mysteriumClient server.Client, dialogCreator DialogCreator, promiseIssuerCreator PromiseIssuerCreator,

This comment has been minimized.

Copy link
@vkuznecovas

vkuznecovas Oct 2, 2018

Contributor

This is starting to have a ridiculous amount of parameters. Maybe it's time to create a struct to hold these params?

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 3, 2018

Author Member

Dont want to touch "connection" package much, while You're refactoring it also

Callback func(*BalanceMessage)
}

// GetMessageEndpoint returns endpoint there to receive messages

This comment has been minimized.

Copy link
@vkuznecovas

vkuznecovas Oct 2, 2018

Contributor

typo "there" -> "where"

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 2, 2018

Author Member

Fixed

}

// Consume handles messages from endpoint
func (consumer *BalanceMessageConsumer) Consume(messagePtr interface{}) error {

This comment has been minimized.

Copy link
@vkuznecovas

vkuznecovas Oct 2, 2018

Contributor

why define the error as return type, when all you're returning is nil?

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 2, 2018

Author Member

Remade callback to be consistent to this interface

@@ -1,5 +1,5 @@
/*

This comment has been minimized.

Copy link
@vkuznecovas

vkuznecovas Oct 2, 2018

Contributor

bad naming, should be balance_consumer_test

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 2, 2018

Author Member

Renamed

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

This comment has been minimized.

Copy link
@vkuznecovas

vkuznecovas Oct 2, 2018

Contributor

bad naming, should be balance_producer

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 2, 2018

Author Member

Renamed

func (receiver *captureReceiver) AfterParse(initArgs seelog.CustomReceiverInitArgs) error {
return nil
}
func (receiver *captureReceiver) Flush() {

This comment has been minimized.

Copy link
@vkuznecovas

vkuznecovas Oct 2, 2018

Contributor

Lint won't be happy with this - do comment on the exported method

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 2, 2018

Author Member

Not exporting this struct

func (receiver *captureReceiver) Flush() {

}
func (receiver *captureReceiver) Close() error {

This comment has been minimized.

Copy link
@vkuznecovas

vkuznecovas Oct 2, 2018

Contributor

Lint won't be happy with this - do comment on the exported method

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 2, 2018

Author Member

Same here

@@ -25,3 +29,11 @@ type Money struct {
func NewMoney(amount float64, currency Currency) Money {
return Money{uint64(amount * 100000000), currency}
}

func (value *Money) String() string {

This comment has been minimized.

Copy link
@vkuznecovas

vkuznecovas Oct 2, 2018

Contributor

Lint won't be happy with this - do comment on the exported method

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 2, 2018

Author Member

Fixed

return respondWithSession(sessionInstance), nil
case ErrorInvalidProposal:
return respondWithError(err.Error()), nil
default:

This comment has been minimized.

Copy link
@vkuznecovas

vkuznecovas Oct 2, 2018

Contributor

aren't we hiding error details here?

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 2, 2018

Author Member

Introduced static error messages

},
)
if subscribeError != nil {
if subscribeError := handler.subscribeSessionRequests(dialog); subscribeError != nil {

This comment has been minimized.

Copy link
@vkuznecovas

vkuznecovas Oct 2, 2018

Contributor

just return handler.subscribeSessionRequests(dialog) ?

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 2, 2018

Author Member

Simplified

&identity.VerifierFake{},
)
}
var _ communication.Codec = NewCodecSecured(

This comment has been minimized.

Copy link
@soffokl

soffokl Oct 2, 2018

Member

NewCodeSecured return codecSecured struct, so we can just use:

var _ communication.Codec = &codecSecured{}

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 2, 2018

Author Member

Fixed

@@ -359,6 +380,21 @@ func (fd *fakeDialog) Request(producer communication.RequestProducer) (responseP
nil
}

type fakePromiseIssuer struct {
started bool

This comment has been minimized.

Copy link
@soffokl

soffokl Oct 2, 2018

Member

started = true equals started
started = false equals stopped
no needs for two variables

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 2, 2018

Author Member

Actually no, I need explicit flag that Stop() was called and started = false does not mean that

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 2, 2018

Author Member

Improved naming

// 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
@soffokl

soffokl Oct 2, 2018

Member

comment from here

where's the speed?

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 2, 2018

Author Member

Improved comment

err := json.Unmarshal([]byte(test.json), &model)

assert.Equal(t, test.expectedModel, model)
if test.expectedError != nil {

This comment has been minimized.

Copy link
@soffokl

soffokl Oct 2, 2018

Member

Whole if block can be replaced by:
assert.Equal(t, err, test.expectedError)

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 3, 2018

Author Member

Incredible insight

Show resolved Hide resolved core/promise/methods/noop/dialog_mock.go
Show resolved Hide resolved core/promise/methods/noop/promise_processor.go

@Waldz Waldz force-pushed the Waldz:feature/promise-balance-notifications branch from 5cba12e to fdf7628 Oct 2, 2018

Waldz added some commits Sep 14, 2018

@Waldz Waldz force-pushed the Waldz:feature/promise-balance-notifications branch 2 times, most recently from 996a10a to 9c5da2b Oct 2, 2018

Waldz added some commits Oct 1, 2018

@Waldz Waldz force-pushed the Waldz:feature/promise-balance-notifications branch from 9c5da2b to a022921 Oct 3, 2018

@soffokl

soffokl approved these changes Oct 3, 2018

@Waldz Waldz merged commit a987494 into mysteriumnetwork:master Oct 3, 2018

1 check passed

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

@Waldz Waldz deleted the Waldz:feature/promise-balance-notifications branch Oct 3, 2018

soffokl added a commit that referenced this pull request Oct 3, 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.