Skip to content

Conversation

@awrichar
Copy link
Contributor

@awrichar awrichar commented Oct 21, 2021

Part of #218.

This basically works, but I'd like to vet the behavior since there are some slight inconsistencies.

When you submit a transfer request, you'll (now) ultimately receive one of two events in response:

  • EventTypeTransferConfirmed if transfer is confirmed -> references the transfer's LocalID so you can look up the transfer
  • EventTypeTransferOpFailed if transfer fails -> references the operation's ID so you can look up the failed operation (there is no transfer object created in this case)

Even though this means the events are a bit asymmetric, I can't come up with a more sensible way to do it.

However, when submitting a transfer with confirm=true, the sync-async bridge tracks a single ID which will ultimately resolve in success or failure. I've chosen the transfer's LocalID, but that means there's an additional step in the case of EventTypeTransferOpFailed, for sync-async to map the operation ID to the transfer LocalID. This also meant extracting some helpers to txcommon (for lack of a better place to put them).

@codecov-commenter
Copy link

codecov-commenter commented Oct 21, 2021

Codecov Report

Merging #279 (d28bc53) into main (bce659a) will decrease coverage by 0.26%.
The diff coverage is 32.65%.

❗ Current head d28bc53 differs from pull request most recent head 819ef6f. Consider uploading reports for the commit 819ef6f to get more accurate results
Impacted file tree graph

@@             Coverage Diff             @@
##              main     #279      +/-   ##
===========================================
- Coverage   100.00%   99.73%   -0.27%     
===========================================
  Files          223      224       +1     
  Lines        12374    12395      +21     
===========================================
- Hits         12374    12362      -12     
- Misses           0       32      +32     
- Partials         0        1       +1     
Impacted Files Coverage Δ
internal/assets/manager.go 100.00% <ø> (ø)
internal/events/operation_update.go 75.00% <0.00%> (-25.00%) ⬇️
internal/txcommon/token_inputs.go 0.00% <0.00%> (ø)
pkg/fftypes/event.go 100.00% <ø> (ø)
internal/syncasync/sync_async_bridge.go 89.84% <37.50%> (-10.16%) ⬇️
internal/assets/token_transfer.go 100.00% <100.00%> (ø)
internal/events/tokens_transferred.go 100.00% <100.00%> (ø)
internal/orchestrator/bound_callbacks.go 100.00% <100.00%> (ø)
internal/tokens/fftokens/fftokens.go 100.00% <100.00%> (ø)

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 bce659a...819ef6f. Read the comment docs.

@awrichar awrichar force-pushed the failtransfer branch 2 times, most recently from a974d8c to d28bc53 Compare October 21, 2021 01:26
Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
@awrichar
Copy link
Contributor Author

I've realized this is incomplete - there's still an issue when a transfer is accompanied by a message, because if the transfer fails, the message recipients will be waiting forever to receive it. Closing this until I can address that problem, since it may impact the shape of this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants