Skip to content
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

Convert TezosKitError to enum type #185

Merged
merged 9 commits into from
Apr 28, 2020
Merged

Convert TezosKitError to enum type #185

merged 9 commits into from
Apr 28, 2020

Conversation

keefertaylor
Copy link
Owner

@keefertaylor keefertaylor commented Mar 22, 2020

Early on in this project I made the (stupid) choice to make errors structs instead of enums. This has lead to problematic code where underlying errors are always optional and are often just untyped strings.

As TezosKit handles more complex interactions, it may be useful to return arrays of errors, typed errors or to know that there is no additional data associated with an error.

This breaking change moves error handling to enums with associated values. In essence, this is a prefactor for #183 / #179.

@keefertaylor keefertaylor changed the title WIP: Convert TezosKitError to enum type Convert TezosKitError to enum type Apr 13, 2020
@codecov
Copy link

codecov bot commented Apr 13, 2020

Codecov Report

Merging #185 into master will decrease coverage by 0.27%.
The diff coverage is 39.72%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #185      +/-   ##
==========================================
- Coverage   53.85%   53.58%   -0.28%     
==========================================
  Files         102      102              
  Lines        3281     3281              
==========================================
- Hits         1767     1758       -9     
- Misses       1514     1523       +9     
Impacted Files Coverage Δ
...romiseKit/TezosNode/TezosNodeClient+Promises.swift 0.00% <0.00%> (ø)
TezosKit/Conseil/ConseilClient.swift 69.87% <0.00%> (ø)
TezosKit/Dexter/DexterExchangeClient.swift 95.48% <0.00%> (ø)
TezosKit/Dexter/TokenContractClient.swift 52.00% <0.00%> (ø)
...TezosNode/Models/Operation/OperationResponse.swift 0.00% <0.00%> (ø)
TezosKit/TezosNode/Services/FeeEstimator.swift 87.20% <0.00%> (+1.37%) ⬆️
TezosKit/TezosNode/TezosNodeClient.swift 33.00% <0.00%> (+0.10%) ⬆️
TezosKit/TezosNode/Services/OperationFactory.swift 81.87% <12.50%> (ø)
...ezosKit/TezosNode/Services/SimulationService.swift 93.02% <66.66%> (+2.11%) ⬆️
...zosKit/TezosNode/Services/RPCResponseHandler.swift 98.07% <95.45%> (+1.99%) ⬆️
... and 4 more

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 d642220...151b3c5. Read the comment docs.

@keefertaylor keefertaylor requested a review from simonmcl April 13, 2020 09:07
}

// Check for \"status\":\"backtracked\" in the response, indicating a failed transaction being rolledback
if stringData.contains("\"status\":\"backtracked\"") {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@keefertaylor can you try take the code I had for catching backtracks in the other PR and replace this block with it. Feel free to refactor again, update, rename etc. But I can't update to the latest code until thats in it as it will break our app. Once you have that you can close my error handling PR entirely.

Also I came across another issue that mine wasn't catching either. If you use a customFeePolicy and set it too low (by accident). Because the estimation is never called it doesn't catch gas errors. When that happens you get a .success(let opHash) back from the tezosKit send, but the console will have "protoxxx.gasExhausted....." in it. Would be great to try catch that too

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acked on the gasExhaused. Filed #190

@keefertaylor
Copy link
Owner Author

Hey @simonmcl,

I added your logic in. I added a new error type, operationError with an associated value of an array of OperationResponseInternalResultError. I did this because we generate these errors inside RPCResponseHandler and this class isn't necessarily tied to transaction formation.

I also added a TODO for myself to add a test here. It should be easy to come up with some fake data and ensure that we parse these errors correctly and that would help us prevent future regressions. If you happen to have an example of a response with one of these errors on hand, I'd be grateful if you could share.

LMK what you think.

@simonmcl
Copy link
Collaborator

@keefertaylor sure, below is the gas exhausted error when using hardcoded gas costs. The structure of the other errors is in this PR: #183 . I replaced some of the large objects with ... but they are not necessary for the error logic

[{"contents":[{"kind":"transaction","source":"tz1e4hAp7xpjekmXnYe4677ELGA3UxR79EFb","fee":"14464","counter":"627017","gas_limit":"139719","storage_limit":"7051","amount":"0","destination":"KT1PuoBCrK7bu9MP7LKZRhjXKZSwvgQkDrCN","parameters":{"entrypoint":"approve","value":{"prim":"Pair","args":[{"string":"KT1PxBTZFsBd1gUXhkTFpncDTjvrKbmTeHde"},{"int":"1"}]}},"metadata":{"balance_updates":[{"kind":"contract","contract":"tz1e4hAp7xpjekmXnYe4677ELGA3UxR79EFb","change":"-14464"},{"kind":"freezer","category":"fees","delegate":"tz1Ke2h7sDdakHJQh8WX4Z372du1KChsksyU","cycle":182,"change":"14464"}],"operation_result":{"status":"backtracked","storage":{"prim":"Pair","args":[{"int":"239"},{"int":"1000000"}]},"big_map_diff":[{"action":"update","big_map":"239","key_hash":"exprtkFpPpLkPNbxReKT1osWCfseKDUyyxb7URVBaX2Yw1N5Sq86hv","key":{"bytes":"0000ca1c5dd7f6665501b57c692f0726a1db46fd1d18"},"value":{"prim":"Pair","args":[[{"prim":"Elt","args":[{"bytes":"01a8969119375713736a2da93ffad0ae2f0178630100"},{"int":"1"}]}],{"int":"228"}]}}],"consumed_gas":"139619","storage_size":"6794"}}},{"kind":"transaction","source":"tz1e4hAp7xpjekmXnYe4677ELGA3UxR79EFb","fee":"62742","counter":"627018","gas_limit":"520845","storage_limit":"21605","amount":"0","destination":"KT1PxBTZFsBd1gUXhkTFpncDTjvrKbmTeHde","parameters":{"entrypoint":"tokenToXtz","value":{"prim":"Pair","args":[{"prim":"Pair","args":[{"prim":"Pair","args":[{"string":"tz1e4hAp7xpjekmXnYe4677ELGA3UxR79EFb"},{"string":"tz1e4hAp7xpjekmXnYe4677ELGA3UxR79EFb"}]},{"prim":"Pair","args":[{"int":"1"},{"int":"39756"}]}]},{"string":"2020-04-27T09:43:41Z"}]}},"metadata":{"balance_updates":[{"kind":"contract","contract":"tz1e4hAp7xpjekmXnYe4677ELGA3UxR79EFb","change":"-62742"},{"kind":"freezer","category":"fees","delegate":"tz1Ke2h7sDdakHJQh8WX4Z372du1KChsksyU","cycle":182,"change":"62742"}],"operation_result":{"status":"backtracked","storage":{"prim":"Pair","args":[{"int":"307"},{"prim":"Pair","args":[{"prim":"Pair","args":[{"prim":"Pair","args":[{"prim":"None"},{"prim":"None"}]},{"prim":"Pair","args":[{"int":"1580637600"},{"int":"10000000"}]}]},{"prim":"Pair","args":[{"bytes":"01a82322859105a5974ef58b465962428c567a204d00"},{"int":"1511"}]}]}]},"consumed_gas":"467921","storage_size":"15430"},"internal_operation_results":[{"kind":"transaction","source":"KT1PxBTZFsBd1gUXhkTFpncDTjvrKbmTeHde","nonce":0,"amount":"39756","destination":"tz1e4hAp7xpjekmXnYe4677ELGA3UxR79EFb","result":{"status":"backtracked","balance_updates":[{"kind":"contract","contract":"KT1PxBTZFsBd1gUXhkTFpncDTjvrKbmTeHde","change":"-39756"},{"kind":"contract","contract":"tz1e4hAp7xpjekmXnYe4677ELGA3UxR79EFb","change":"39756"}],"consumed_gas":"10207"}},{"kind":"transaction","source":"KT1PxBTZFsBd1gUXhkTFpncDTjvrKbmTeHde","nonce":1,"amount":"0","destination":"KT1PuoBCrK7bu9MP7LKZRhjXKZSwvgQkDrCN","parameters":{"entrypoint":"default","value":{"prim":"Right","args":[{"prim":"Pair","args":[{"prim":"Pair","args":[{"bytes":"0000ca1c5dd7f6665501b57c692f0726a1db46fd1d18"},{"bytes":"01a8969119375713736a2da93ffad0ae2f0178630100"}]},{"int":"1"}]}]}},"result":{"status":"failed","errors":[{"kind":"temporary","id":"proto.006-PsCARTHA.gas_exhausted.operation"}]}}]}}],"signature":"edsigtzVs2YoRGaZoDrvug9jKJz7KY7pbJymWqvG2QWxaM3SMECHHNVGEqRAAhAcBnXWBnNk7fzPVA6We1e644Qb2oZxgCR1cYy"}]

@simonmcl
Copy link
Collaborator

Code looks good. I'll give it all a proper test in the app once its all merged. I have some transaction history code that needs to be merged in after

@keefertaylor
Copy link
Owner Author

Cool. I'm going to land this code as is and then follow up on the gas exhausted stuff.

@keefertaylor keefertaylor merged commit f45f2c0 into master Apr 28, 2020
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