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

fix(xyk)!: change xyk pallet errors #284

Merged

Conversation

nasqn
Copy link

@nasqn nasqn commented Jun 21, 2021

Description

AssetBalanceLimitExceeded is splitted into two errors, one if limit exceeded and one if limit not reached.

Fixes: #275

Motivation and Context

There was only one error for two different cases and error name itself was not accurate. Two different errors helps to better indicate what's the problem and when it actually occurs.

How Has This Been Tested?

Run tests locally, fixed the broken one.

Checklist:

  • [v ] I have updated the documentation if necessary.
  • I have added tests to cover my changes, regression test if fixing an issue.
  • [v ] This is a breaking change.

@nasqn
Copy link
Author

nasqn commented Jun 21, 2021

Why do we need specific errors even for very similar situations?

for example, instead of:

		/// Overflow
		AddAssetAmountInvalid, // no tests
		/// Overflow
		RemoveAssetAmountInvalid, // no tests
		/// Overflow
		SellAssetAmountInvalid, // no tests
		/// Overflow
		BuyAssetAmountInvalid, // no tests
		/// Overflow
		FeeAmountInvalid, // no tests

we could have only AssetAmountInvalid and use it in all cases.

I'm asking, because I have the similar situation - exceeding the limit in add_liquidity and buy functions. So I added more common error AssetAmountExceededLimit and use it in both places

Or should I add one more error for add_liquidity function and give more accurate names?

@enthusiastmartin
Copy link
Contributor

Why do we need specific errors even for very similar situations?

for example, instead of:

		/// Overflow
		AddAssetAmountInvalid, // no tests
		/// Overflow
		RemoveAssetAmountInvalid, // no tests
		/// Overflow
		SellAssetAmountInvalid, // no tests
		/// Overflow
		BuyAssetAmountInvalid, // no tests
		/// Overflow
		FeeAmountInvalid, // no tests

we could have only AssetAmountInvalid and use it in all cases.

I'm asking, because I have the similar situation - exceeding the limit in add_liquidity and buy functions. So I added more common error AssetAmountExceededLimit and use it in both places

Or should I add one more error for add_liquidity function and give more accurate names?

I agree that this could be simplified to have only one error. It was over-enthusiastic approach when the pallet was initially implemented.

I believe that the specific error wont be very helpful anyway. You can see which extrinsic it has been raised for.

These errors are always returned when there is problem in calculation, an arithmetic problem - so i would also consider adding simple 'Overflow' error for all these cases.

@nasqn nasqn marked this pull request as ready for review June 26, 2021 11:43
@nasqn
Copy link
Author

nasqn commented Jun 26, 2021

So, is current implementation OK?

@enthusiastmartin enthusiastmartin merged commit 18b05cb into galacticcouncil:master Jun 28, 2021
@nasqn nasqn deleted the fix/change_xyk_pallet_errors branch June 28, 2021 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

change: xyk limit error name not accurate
3 participants