-
Notifications
You must be signed in to change notification settings - Fork 535
Algod: Additional simulation result information #4439
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
Changes from 203 commits
8f840dc
62f4b59
505bc50
58846a3
e242bc4
63805af
5d93ceb
2c8c912
b9f96ee
e52d1bf
a9f4d1a
8a54d2e
301a366
06d0063
0631333
7f0b564
6e5c9ce
64567aa
fc76cb2
760bbfb
cc6e605
c51926f
deb330f
19cdb73
462a9dd
b2c4f51
6ec12f1
b5473ae
307709d
7995700
02d0947
a3e0f59
dad0197
ac72228
50304a3
33dda59
f1c0b05
f90c19a
394f7e7
be38467
970d9f4
595ca6b
5f45dfe
7897a2e
051cbef
65cba5c
b78e899
7324c0e
ad3be73
2176d2d
cb381d8
195ac07
e2eb402
2d92ad4
2f189d2
5bf32e7
c7996d3
08f8e5e
f16d378
83fae5c
2a30a89
f32981a
8c90301
251cc15
9bbd757
b9acbf7
0c99043
d91c4a6
9700a89
b5bb33f
6591780
ea919e9
4fa3eef
3165675
4351fd6
9067f70
e22579b
6e448a8
139672a
6ad7387
47e83dc
ba5d43e
82f31c8
d3e4253
bf3907b
1e518be
5da2cd8
e7ce7fc
6e106ba
75d5326
dd27558
4cf359e
67e0978
8c4310b
4a2a998
042704a
ca59421
a0a0550
6673103
488d748
ce98909
8bdbd5f
d7981a8
82b75e9
959261f
25899d0
1908046
0aca986
827d4e2
e5fd2a1
b99b80b
ad5f515
2a2b8a9
115421f
de27a6c
533c239
1219bef
361931f
8a75f1b
983ea77
74b4f96
78f3892
1c7c406
807b384
3f4e74e
4a74a24
2f083e8
3c5bc99
70ddd43
6de00a4
c2dfb80
a76ce0c
3ef9112
f153f4a
d1f0d0e
845425b
075e288
b1ee178
336038e
f3ea97e
987fd51
a932450
b20f6a7
cfe8496
0e19762
76ca4cd
82d3f2c
942901c
84c5dff
446d158
3d57840
f588511
8b4384b
0c43566
54b7534
5927a63
0b02d9e
10e74cb
c0489c7
f745436
9be7012
d087b0a
e515ebd
46d0a06
df139a9
73d2efa
b4e0999
66b501d
5eb4820
ed84777
9d60dd7
d3e5075
58d8982
7877000
58dba87
00daa4e
020502e
332da4c
ac2883d
b51e198
8ec4b67
e6575eb
4f8d9ba
51f4877
1f25626
3ae89a9
f373eb9
09694a7
1ab4254
c64d018
f4d92b8
ffaa3a2
bcb7ec8
7b8ee52
d92088d
6443f4a
aea3f68
777bf69
08f99a0
4476bbe
bbc03ba
f060ccf
00e8660
cc7be80
b737c49
88b7182
689d686
5311c6f
865cafc
aa32610
9557a66
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1215,6 +1215,7 @@ | |
| "application/x-binary" | ||
| ], | ||
| "produces": [ | ||
| "application/json", | ||
| "application/msgpack" | ||
| ], | ||
| "schemes": [ | ||
|
|
@@ -1232,6 +1233,9 @@ | |
| "type": "string", | ||
| "format": "binary" | ||
| } | ||
| }, | ||
| { | ||
| "$ref": "#/parameters/format" | ||
| } | ||
| ], | ||
| "responses": { | ||
|
|
@@ -3321,6 +3325,49 @@ | |
| } | ||
| } | ||
| }, | ||
| "SimulationTransactionGroupResult": { | ||
| "description": "Simulation result for an atomic transaction group", | ||
| "type": "object", | ||
| "required": [ | ||
| "txn-results" | ||
| ], | ||
| "properties": { | ||
| "txn-results": { | ||
| "description": "Simulation result for individual transactions", | ||
| "type": "array", | ||
| "items": { | ||
| "$ref": "#/definitions/SimulationTransactionResult" | ||
| } | ||
| }, | ||
| "failure-message": { | ||
| "description": "If present, indicates that the transaction group failed and specifies why that happened", | ||
| "type": "string" | ||
| }, | ||
| "failed-at": { | ||
| "description": "If present, indicates which transaction in this group caused the failure", | ||
| "type": "array", | ||
| "items": { | ||
| "type": "integer" | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "SimulationTransactionResult": { | ||
| "description": "Simulation result for an individual transaction", | ||
| "type": "object", | ||
| "required": [ | ||
| "txn-result" | ||
| ], | ||
| "properties": { | ||
| "txn-result": { | ||
| "$ref": "#/definitions/PendingTransactionResponse" | ||
| }, | ||
| "missing-signature": { | ||
| "description": "A boolean indicating whether this transaction is missing signatures", | ||
| "type": "boolean" | ||
| } | ||
| } | ||
| }, | ||
| "StateProof": { | ||
| "description": "Represents a state proof and its corresponding message", | ||
| "type": "object", | ||
|
|
@@ -3485,7 +3532,7 @@ | |
| "msgpack" | ||
| ], | ||
| "type": "string", | ||
| "description": "Configures whether the response object is JSON or MessagePack encoded.", | ||
| "description": "Configures whether the response object is JSON or MessagePack encoded. If not provided, defaults to JSON.", | ||
| "name": "format", | ||
| "in": "query" | ||
| }, | ||
|
|
@@ -3972,16 +4019,24 @@ | |
| "schema": { | ||
| "type": "object", | ||
| "required": [ | ||
| "failure-message", | ||
| "missing-signatures" | ||
| "version", | ||
| "txn-groups", | ||
| "would-succeed" | ||
| ], | ||
| "properties": { | ||
| "failure-message": { | ||
| "description": "\\[fm\\] Failure message, if the transaction would have failed during a live broadcast.", | ||
| "type": "string" | ||
| "version": { | ||
| "description": "The version of this response object.", | ||
| "type": "integer" | ||
| }, | ||
| "txn-groups": { | ||
| "description": "A result object for each transaction group that was simulated.", | ||
| "type": "array", | ||
| "items": { | ||
| "$ref": "#/definitions/SimulationTransactionGroupResult" | ||
| } | ||
| }, | ||
| "missing-signatures": { | ||
| "description": "\\[ms\\] Whether any transactions would have failed during a live broadcast because they were missing signatures.", | ||
| "would-succeed": { | ||
| "description": "Indicates whether the simulated transactions would have succeeded during an actual submission.", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suppose this is true if all of the transaction groups would have succeeded? I think I'd just drop it, but I don't feel strongly. It seems a bit strange that we have three levels of results being aggregated, Txn, TxnGroup, and Group of TxnGroups. At each level, we describe success a little differently. At the transaction level, we put it inside the txn-result, then at the group level, we aggregate it as a "failure-message", and then at the very top, we aggregate the other way, as "would-succeed". We could strive for more unity here, or we could skip the aggregation entirely.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The motivation behind "would-succeed" at this level was to provide a dead simple way of figuring out if your transactions would succeed if you sent them to the real submission endpoint. Yes, this is redundant information, since you could deduce the same thing by checking that no TxnGroup has a failure message and no Txn is missing signatures. But in the future you may have to check more things, like no additional foreign resources were needed and you didn't exceed the standard opcode budget. True, these features would likely be opt-in so they're less likely to confuse people, but still think there's a benefit to summarizing all the possible reasons for failure in a simple boolean.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm ok with this, but I'm actually so convinced that I think it should exist on individual txn-groups as well (we can add in a later PR, since it will be an addition). The idea that there will be multiple ways that we might allow a group to "succeed" even though they would not actually succeed on chain is what convinces me. And, I think there will be a good use for simulation in unit tests where you want to confirm that, say, txgroups 1, 2 , 3 and 5 would succeed, but 4 would not. That is, the convenience you're offering is good at the txgroup level, because you may want some of the groups to fail (you are testing that they will, in fact). But that the other groups are perfectly correct.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Those are good points. I'll admit I haven't given much thought to how multiple groups would be handled, but your ideas make sense. |
||
| "type": "boolean" | ||
| } | ||
| } | ||
|
|
||
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.
Why is this an array? The description appears to say it will be a single integer.
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.
It's an array because we show you the path to the failing txn. E.g. if the second txn is an app call whose third inner failed,
failed-atwould be[1,2]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.
Can we explain that succinctly(?) in the description? Perhaps it's too much.
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 made an attempt in 9557a66, let me know what you think
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 think including that in the description is nice - the endpoint only returns the first error so it could be ambiguous whether
failed-atis reporting every error in the group, or the path to the failed transaction.