-
Notifications
You must be signed in to change notification settings - Fork 242
Only send transfer message if token transfer is successful #297
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
Conversation
Codecov Report
@@ Coverage Diff @@
## main #297 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 224 225 +1
Lines 12387 12421 +34
=========================================
+ Hits 12387 12421 +34
Continue to review full report at Codecov.
|
912536c to
21a9016
Compare
Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
The message is now stashed away on the Operation inputs, then loaded and sent by the event manager if the transfer succeeds. Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
21a9016 to
eda9995
Compare
docs/swagger/swagger.yaml
Outdated
| type: array | ||
| state: | ||
| enum: | ||
| - notready |
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.
No big objectection to notready, but in general I have a bell that pings when I see a negative.
Wonder if any of these would work?
stagedblockedpreprocessinginitializing
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.
Yea, actually staged was my first take on this. Maybe should go back to that.
peterbroadhurst
left a comment
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.
Looks great 👍
One minor spelling related question to think about, and see if you want to make a change or not.
| } | ||
|
|
||
| // Special handling for OpTypeTokenTransfer, which writes an event when it fails | ||
| if op.Type == fftypes.OpTypeTokenTransfer && txState == fftypes.OpStatusFailed { |
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.
Mental note (not suggesting any change): the next time we find this pattern, we might want to do something a little generic around failure event mapping, rather than building a bunch of switches down here.
peterbroadhurst
left a comment
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.
👍 marking approval here to allow merging when the spelling change and. merge conflicts are done.
Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
This is a rework of #279 - please see the original notes on that PR, which still apply here.
Falls under architecture item #218.
It contains the commits from #283 and #296 as pre-requisites, so those should be reviewed first. Only the last 2 commits are net new code.