-
Notifications
You must be signed in to change notification settings - Fork 27
Conversation
a14d0cf
to
8d8b30e
Compare
1e909e7
to
900ddd9
Compare
7c115c4
to
9235f52
Compare
to confirm: on messages that fail to apply we get a null message receipt, but no other encoding within the test-vector json of the error code or other things around message failure? |
Correct. Although, looks like this may not end up being the case... filecoin-project/lotus#3697
Under usual circumstances the receipt will carry the exit/error code. Actors are expected to call |
}) | ||
|
||
// am.Result may still be nil if the message failed to be applied | ||
if am.Result != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a more idiomatic way of doing this would be:
var receipt *schema.Receipt
if am.Result != nil {
receipt = &schema.Receipt{
ExitCode: int64(am.Result.ExitCode),
ReturnValue: am.Result.Return,
GasUsed: am.Result.GasUsed,
}
}
b.vector.Post.Receipts = append(b.vector.Post.Receipts, receipt)
traces := make([]types.ExecutionTrace, 0, len(msgs)) | ||
for _, msgs := range msgs { | ||
traces = append(traces, msgs.Result.ExecutionTrace) | ||
for _, msg := range msgs { | ||
if msg.Result != nil { | ||
traces = append(traces, msg.Result.ExecutionTrace) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we want to make the traces a slice of pointers []*types.ExecutionTrace
, and leave a nil element for each message that doesn't have a result?
@@ -103,7 +103,7 @@ func (b *MessageVectorBuilder) CommitApplies() { | |||
|
|||
for _, am := range b.Messages.All() { | |||
// apply all messages that are pending application. | |||
if am.Result == nil { | |||
if !am.Applied { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, this is a lot more explicit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apply_message_failures in vectors - do we still want to do that?
I think we want to be as declarative and explicit as possible. After all, a general goal of test vectors is to remove ambiguity and uncertainty. For machines, it might be redundant to have this index, but I anticipate that this explicit acknowledgement of which messages fail will be useful for when humans have to debug something.
Related: I like what you did with ApplicableMessage#Applied
and I'm thinking we should add a Failed
field to ApplicableMessage
, and use that instead of nil checks everywhere. A nil can mean a lot of things, but Failed: true
can only mean one thing.
We can also replace the Nil()
predicate by a Failed()
predicate, which makes the API a lot more semantic and self-descriptive, IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is OK and I'm happy to merge as-is. I'll open a follow-up issue to try to address the comments and the TODO items!
TODO:
apply_message_failures
in vectors - do we still want to do that? (context)null
receiptsDepends on:
Upstream fixes for broken tests (not blocking merge):
resolves #87
resolves #15
closes #49