Move definitions of datatypes and tokens to their proper namespace #312

nguyer · 2021-11-01T19:51:57Z

Messages that use the ff_system namespace:

Org registration
Node registration
Namespace definition

Messages that use whatever namespace is specified in the API request path:

Everything else

Signed-off-by: Nicko Guyer <[email protected]>

awrichar · 2021-11-01T19:57:21Z

internal/events/aggregator.go

 	eventType := fftypes.EventTypeMessageConfirmed
 	switch {
-	case msg.Header.Namespace == fftypes.SystemNamespace:
+	case msg.Header.Namespace == fftypes.SystemNamespace || msg.Header.Type == fftypes.MessageTypeDefinition:


Does syshandler now handle anything that is not a definition?

I think it's only ever handled definitions

I wonder if it needs a different name 🙃

And maybe the first part of the if clause here is irrelevant...

Possibly. Probably worth some input from @peterbroadhurst on this one. I'll try to chat with him when he gets a chance. The rest of the code, including the unit tests seem to be written in a way that assumes there could be system broadcasts that are not definitions. Maybe there is a case I'm not thinking about, possibly a future case. Or maybe the way the code was originally written is not relevant anymore.

Sounds good. I've approved since I think these changes are good - but probably need to have a chat about the future of what else may be on the ff_system namespace, and if "syshandler" is really the right name for the thing that is handling definitions.

@peterbroadhurst I removed the first part of this case in the switch statement, which did change some failure code paths in the tests. I'll highlight those in the places that changed. But would appreciate your eyes on this to make sure the correct thing is still happening here.

test/e2e/tokens_test.go

Signed-off-by: Nicko Guyer <[email protected]>

codecov-commenter · 2021-11-11T21:28:20Z

Codecov Report

Merging #312 (1ff29f8) into main (29fe427) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #312   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          232       234    +2     
  Lines        12720     12788   +68     
=========================================
+ Hits         12720     12788   +68

Impacted Files	Coverage Δ
internal/broadcast/manager.go	`100.00% <ø> (ø)`
internal/broadcast/datatype.go	`100.00% <100.00%> (ø)`
internal/broadcast/definition.go	`100.00% <100.00%> (ø)`
internal/broadcast/namespace.go	`100.00% <100.00%> (ø)`
internal/broadcast/tokenpool.go	`100.00% <100.00%> (ø)`
internal/definitions/definition_handler.go	`100.00% <100.00%> (ø)`
...nternal/definitions/definition_handler_datatype.go	`100.00% <100.00%> (ø)`
...ternal/definitions/definition_handler_namespace.go	`100.00% <100.00%> (ø)`
...nal/definitions/definition_handler_network_node.go	`100.00% <100.00%> (ø)`
...rnal/definitions/definition_handler_network_org.go	`100.00% <100.00%> (ø)`
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 29fe427...1ff29f8. Read the comment docs.

nguyer · 2021-11-11T21:31:29Z

internal/events/aggregator_test.go

+	mdm.On("ValidateAll", ag.ctx, mock.Anything).Return(false, nil)
+
+	dbm := ag.database.(*databasemocks.Plugin)
+	dbm.On("UpdateMessage", ag.ctx, mock.Anything, mock.Anything).Return(nil)
+	dbm.On("InsertEvent", ag.ctx, mock.Anything).Return(errors.Errorf("pop"))


@peterbroadhurst This is one of the tests I had to change

nguyer · 2021-11-11T21:32:03Z

internal/events/aggregator_test.go

-	msh := ag.syshandlers.(*syshandlersmocks.SystemHandlers)
-	msh.On("HandleSystemBroadcast", mock.Anything, mock.Anything, mock.Anything).Return(false, nil)
-
 	mdm := ag.data.(*datamocks.Manager)
 	mdm.On("GetMessageData", ag.ctx, mock.Anything, true).Return([]*fftypes.Data{}, true, nil)

+	mdm.On("ValidateAll", ag.ctx, mock.Anything).Return(false, nil)
+


@peterbroadhurst This is one of the tests I had to change

Just to check, why wouldn't you just change msh := ag.syshandlers.(*syshandlersmocks.SystemHandlers) to be the DefinitionHandlers, and return false and set Header.Type on the message in attemptMessageDispatch?

e.g. so you're testing the same path.

peterbroadhurst · 2021-11-11T21:38:12Z

internal/events/aggregator.go

 	eventType := fftypes.EventTypeMessageConfirmed
 	switch {
-	case msg.Header.Namespace == fftypes.SystemNamespace:
+	case msg.Header.Type == fftypes.MessageTypeDefinition:


This looks right to me 👍
Think this is the substantial change, matching the package rename.
We went from "If it's on the system namespace, the system handlers handle it"...
To "If it's a definition, then the definition handlers handle it"

Signed-off-by: Nicko Guyer <[email protected]>

nguyer · 2021-11-12T15:50:01Z

Re-running the build because it failed due to this bug: #265

nguyer · 2021-11-12T15:50:13Z

Re-running the build because it failed due to this bug: #265

awrichar · 2021-11-16T20:25:18Z

internal/definitions/definition_handler.go

+		return dh.handleTokenPoolBroadcast(ctx, msg, data)
 	default:
-		l.Warnf("Unknown topic '%s' for system broadcast ID '%s'", msg.Header.Tag, msg.Header.ID)
+		l.Debugf("Unknown topic '%s' for system broadcast or definition ID '%s'", msg.Header.Tag, msg.Header.ID)


This is really only for definitions now, right? The "or" seems superfluous. Also we say "topic" but actually log the "tag".

awrichar · 2021-11-16T20:26:01Z

internal/events/aggregator.go

 	switch {
-	case msg.Header.Namespace == fftypes.SystemNamespace:
+	case msg.Header.Type == fftypes.MessageTypeDefinition:
 		// We handle system events in-line on the aggregator, as it would be confusing for apps to be


"system events" is now more accurately "definition events"

awrichar

New changes look great. Noted a few spelling items in logs/comments.

Signed-off-by: Nicko Guyer <[email protected]>

awrichar · 2021-11-24T18:53:41Z

Looks good, thanks for making those adjustments.

Move definitions of datatypes and tokens to their proper namespace

d52ef6e

Signed-off-by: Nicko Guyer <[email protected]>

nguyer requested review from awrichar and peterbroadhurst as code owners November 1, 2021 19:51

awrichar reviewed Nov 1, 2021

View reviewed changes

test/e2e/tokens_test.go Outdated Show resolved Hide resolved

Add comments to new events in E2E tests

f7e8afc

Signed-off-by: Nicko Guyer <[email protected]>

awrichar approved these changes Nov 2, 2021

View reviewed changes

Rename syshandlers to definitions

7c8bd15

Signed-off-by: Nicko Guyer <[email protected]>

nguyer commented Nov 11, 2021

View reviewed changes

peterbroadhurst reviewed Nov 11, 2021

View reviewed changes

nguyer added 3 commits November 11, 2021 16:41

Merge branch 'main' into namespace-definition

0bfecf2

Signed-off-by: Nicko Guyer <[email protected]>

Update tests and mocks

a9e65e5

Signed-off-by: Nicko Guyer <[email protected]>

Improve codestyle

6f0498f

Signed-off-by: Nicko Guyer <[email protected]>

awrichar reviewed Nov 16, 2021

View reviewed changes

awrichar approved these changes Nov 16, 2021

View reviewed changes

nguyer added 2 commits November 24, 2021 10:32

Merge branch 'main' into namespace-definition

b5abfc4

Signed-off-by: Nicko Guyer <[email protected]>

Update log message and comments for definitions

1ff29f8

Signed-off-by: Nicko Guyer <[email protected]>

nguyer merged commit d03b868 into main Nov 24, 2021

nguyer deleted the namespace-definition branch November 24, 2021 19:02

Move definitions of datatypes and tokens to their proper namespace #312

Move definitions of datatypes and tokens to their proper namespace #312

Uh oh!

Conversation

nguyer commented Nov 1, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov-commenter commented Nov 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nguyer commented Nov 12, 2021

Uh oh!

nguyer commented Nov 12, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

awrichar left a comment

Choose a reason for hiding this comment

Uh oh!

awrichar commented Nov 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-commenter commented Nov 11, 2021 •

edited

Loading