Factor message packaging out of plumbing into core.Outbox #2795
Conversation
| } | ||
|
|
||
| // Send marshals and sends a message, retaining it in the outbound message queue. | ||
| func (ob *Outbox) Send(ctx context.Context, from, to address.Address, value *types.AttoFIL, |
There was a problem hiding this comment.
This is a fairly direct move from msg.Sender, but with tighter dependencies and the message pool/publish extracted to publisher.
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestOutbox(t *testing.T) { |
There was a problem hiding this comment.
These are ported from the msg.Sender tests, but with fakes instead of real dependencies (chain store, state HAMT etc).
I dropped a few internal tests for nextNonce and added some nonce checks to the existing tests, so this test is now in a _test package.
| @@ -119,7 +119,7 @@ type Node struct { | |||
| // Incoming messages for block mining. | |||
| MsgPool *core.MessagePool | |||
| // Messages sent and not yet mined. | |||
| Outbox *core.MessageQueue | |||
| MsgQueue *core.MessageQueue | |||
There was a problem hiding this comment.
I'll replace this with the actual outbox object soon.
node/actor_provider.go
Outdated
| if err != nil { | ||
| return nil, errors.Wrapf(err, "no actor at address %s", addr) | ||
| } | ||
| return actr, nil |
There was a problem hiding this comment.
With regard to using the chainFacade instead of chainProvider and actorProvider, this method is identical to plumbing/bcf/blockchain_facade.go line 89.
| if err = p.network.Publish(p.topic, encoded); err != nil { | ||
| return errors.Wrap(err, "failed to publish message to network") | ||
| } | ||
| return nil |
There was a problem hiding this comment.
Looks great, but I'm not totally clear on why this is under node. Why not put it with the outbox? It feels like the node directory has been sort of devolving into a "misc" folder lately.
There was a problem hiding this comment.
This is a package-level dependency injection. The outbox doesn't know or care how messages are published to the network. This change removed the import of net/pubsub by the core package.
It's "wiring" which definitely feels miscellaneous but is what the node package alone should be doing.
There was a problem hiding this comment.
Couldn't find anything wrong. I like that we're moving this code out of plumbing. I'm with @rosalinekarr that I'm a little uneasy adding code to the node package, but I see the reasoning.
This is part of follow-up refactoring to the message queue that @acruikshank and I discussed a while ago. Sorry it's a little long: the delta is mostly moving and simplifying the old Sender test to exercise the outbox instead.
I intend to also:
Addcallers now knows height)