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(exchange): change pallet errors #271

Merged

Conversation

nasqn
Copy link

@nasqn nasqn commented Jun 2, 2021

Description

AssetBalanceLimitExceeded is splitted into two errors, one for sell case and one for buy.

Fixes: #268

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 2, 2021

This issue looked very simple, but when it's about namings of public interfaces - you should understand all code around it pretty well to give good names.

I was trying to figure out how exchange pallete works, but it's not too easy :) so I'm not sure that I've solved the issue correctly.

@nasqn
Copy link
Author

nasqn commented Jun 2, 2021

Also I don't know is changing of errors a breaking change?

@nasqn nasqn changed the title Fix/exchange pallet errors fix(exchange): change pallet errors Jun 2, 2021
@auto-add-label auto-add-label bot added the bug label Jun 2, 2021
pallets/exchange/src/lib.rs Outdated Show resolved Hide resolved
pallets/exchange/src/lib.rs Outdated Show resolved Hide resolved
@enthusiastmartin enthusiastmartin self-requested a review June 2, 2021 14:46
@enthusiastmartin
Copy link
Contributor

This looks ok , except the new IncorrectMatch error.

However, there is a bug in this area #274 ( fix coming soon ) which will likely to make some changes in this area.

So, let's first fix that bug and then make changes in this PR.

@unordered-set
Copy link
Contributor

Also I don't know is changing of errors a breaking change?

Yes, it is. Ui has some expectations about error names to display valid translations etc. One you change it, fronted needs to be updated accordingly. So you also need to update the palette version

@jak-pan
Copy link
Contributor

jak-pan commented Jun 7, 2021

Also I don't know is changing of errors a breaking change?

Yes, it is. Ui has some expectations about error names to display valid translations etc. One you change it, fronted needs to be updated accordingly. So you also need to update the palette version

Actually, UI shows errors based on /// documentation for error it gets from the node. But it's indeed a breaking change since the deposited events need to be deserialized differently now and that means we need to increase pallet version and also spec_version and runtime version.

@nasqn nasqn force-pushed the fix/exchange_pallet_errors branch from 2dd1beb to d1c4fd8 Compare June 8, 2021 19:29
@nasqn nasqn force-pushed the fix/exchange_pallet_errors branch from d1c4fd8 to c91fee0 Compare June 10, 2021 14:43
@enthusiastmartin enthusiastmartin merged commit a0dfc31 into galacticcouncil:master Jun 10, 2021
@nasqn nasqn deleted the fix/exchange_pallet_errors branch June 10, 2021 18:29
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.

bug: Exchange pallet error incorrect and inacurate
4 participants