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

Error codes #550

Closed
ArtemGr opened this issue Sep 23, 2019 · 6 comments
Closed

Error codes #550

ArtemGr opened this issue Sep 23, 2019 · 6 comments
Assignees
Labels
enhancement New feature or request

Comments

@ArtemGr
Copy link

ArtemGr commented Sep 23, 2019

«we need a standardized error message format like {error: {code: 3, message: "This is the error message", backtrace: "backtrace info for mm2 devs"}} as opposed to random length strings that make it hard for GUI devs to parse around them in order to tell the user whats going on
So - don't assign a specific error code to every error - but please ensure the error output is in a proper format similar to the above proposal. It would be a huge help for us and other GUI devs.»
( discussion: https://discordapp.com/channels/@me/542973365846016001/625604148783480852 )

AG: We might want to
a) Add a macro that would be used across the board to emit an expected error.
For example: ecode! (EL06, "No Electrum servers reachable").
The macro emits a structure that implements Display. It should also be possible to use that structure with human-readable log and dashboard, where error code is printed as a log tag.
b) Add a (build.rs) parser that will go over the source code and check the error code uniqueness, generating a file that lists all the error codes.
This file can then be used by the GUI developers as an extra source of information on the errors and codes and by the MM developers to help them select new error codes.
c) Patch the RPC error handler to parse the error string, separating backtrace and error code tags into the respective separate fields.
(Let's talk the advantages if there's a better approach)

@ArtemGr ArtemGr added the enhancement New feature or request label Sep 23, 2019
@artemii235
Copy link
Member

What if we consider using separated text tags e.g. mm2.lp_ordermatch.utxo.not_sufficient_balance similar to JSON path notation? I think it has advantages in comparison to very common and widespread numeric codes:

  1. Text tag contains info about error itself. Developers do not have to store the mapping in their memory e.g. 20 means that address does not have enough balance to place an order.
  2. We can use stack trace as tags so error path will point to code location returned the error.
  3. Error processing may be very flexible, e.g. one can use *.not_sufficient_balance to process all possible not_sufficient_balance cases or *.utxo.not_sufficient_balance and *.eth.not_sufficient_balance to have different error messages for UTXO and ETH coins.

@ArtemGr
Copy link
Author

ArtemGr commented Mar 18, 2020

Sounds good!

@artemii235
Copy link
Member

We're going to start improving the error structure with trade_preimage RPC. @sergeyboyko0791 is on it right now.

artemii235 pushed a commit that referenced this issue May 3, 2021
* Add and implement MmError

* Add WithdrawError returned from 'MmCoin::withdraw'
* Collect 'available' and 'required' info within GenerateTxError
* Add other internal error types

* Add BalanceError returned from my_balance

* Add more error types
* Add Web3RpcError,
* Refactor the ordered_mature_unspents, list_unspent_ordered methods to return an extended error
* Rename MmRpcError to UtxoRpcError

* Refactor MmError mapping
* Separate mm_error into several modules
* Rename into_mm_and() to into_mm()
* add into_mm_fut

* Implement MarketMaker2 RPC protocol v2
* Add lp_protocol, dispatcher_v2, dispatcher_legacy modules
* Change the coins::withdraw function signature
* Add HttpStatusCode trait
* Refactor test_withdraw_and_send to use v2 protocol
* Add test_mm2rpc test_withdraw_legacy, test_withdraw_not_sufficient_balance

* Upgrade the trade_preimage RPC call to the mmrpc 2.0
* Move lp_swap::trade_preimage and dependent types to the separated lp_swap::trade_preimage module
* Change the trade_preimage_rpc function signature
* Extend CheckBalanceError and move it to the separated check_balance module
* Refactor test_maker_trade_preimage, test_taker_trade_preimage to use v2 protocol
* Add test_trade_preimage_not_sufficient_balance, test_trade_preimage_legacy, test_trade_preimage_dynamic_fee_not_sufficient_balance tests

* Remove boilerplate
* Add lp_coinfind_or_err and CoinFindError
* Replace lp_coinfind with lp_coinfind_or_err in coins::withdraw, trade_preimage_rpc

* Move functions that check the trade balance to check_balance module
* Move check_my_coin_balance_for_swap, check_other_coin_balance_for_swap, check_base_coin_balance_for_swap

* Make a compile time checking if an error type is tagged
* Add ser_error, ser_error_derive crates
* Remove a runtime checking if an error type is tagged
* Fill out the mm_error documentation with the examples

* Fix rustfmt warnings

* Add missing ser_error, ser_error_derive crates

* Fix PR issues
* Rename 'into_mm' to 'map_to_mm', 'into_mm_fut' to 'map_to_mm_fut'
* Remove CoinFindError::NoCoinsContext error variant, replace it with the panic
@artemii235
Copy link
Member

@sergeyboyko0791 The PR is merged, please document the changes in the API 🙂

@sergeyboyko0791
Copy link

I guess we can close this issue?

@artemii235
Copy link
Member

Yes 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants