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

chore: implement an allow list on market for the sell side #11731

Merged
merged 1 commit into from
Oct 17, 2024

Conversation

voltaireChocolatine
Copy link
Contributor

Hi vega team, we've implemented a small feature to only allow a limited amount of key to be allowed to place order on the sell side of the book. This will be useful in some future markets that we plan to implement on nebula.

I realise we could keep this feature on our fork, but it would make life easier for our validators and for future maintenance if this was added in the main repo.

Happy to discuss further the reasons for the changes.

Copy link
Member

@candida-d candida-d left a comment

Choose a reason for hiding this comment

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

@voltaireChocolatine I’ll leave the feature review to someone else but I’ve corrected some doc strings for the APIs

datanode/gateway/graphql/schema.graphql Outdated Show resolved Hide resolved
datanode/gateway/graphql/schema.graphql Outdated Show resolved Hide resolved
datanode/gateway/graphql/schema.graphql Outdated Show resolved Hide resolved
datanode/gateway/graphql/schema.graphql Outdated Show resolved Hide resolved
datanode/gateway/graphql/schema.graphql Outdated Show resolved Hide resolved
datanode/gateway/graphql/schema.graphql Outdated Show resolved Hide resolved
protos/sources/vega/vega.proto Outdated Show resolved Hide resolved
protos/sources/vega/vega.proto Outdated Show resolved Hide resolved
@wwestgarth
Copy link
Contributor

wwestgarth commented Oct 8, 2024

A few things:

  • have you considered AMM's, and that someone could create an AMM to be able to indirectly sell?
  • is it really the intention that people will not even be able to close out their long positions ever? Or is it that you actually only want to stop a party having a short, negative position?
  • this PR does not contain a single unit-test or feature test, are you sure it works?

@voltaireChocolatine
Copy link
Contributor Author

* have you considered AMM's, and that someone could create an AMM to be able to indirectly sell?

No, but I will remove the implementation of this feature for future markets as we do not plan to use it with these, and thinking about it, it doesn't makes sense at all in the context of future markets (I believe AMM as well are only available with future markets?).

* is it really the intention that people will not even be able to close out their long positions ever? Or is it that you actually only want to stop a party having a short, negative position?

I think base on my previous sentence, this is being answered.

* this PR does not contain a single unit-test or feature test, are you sure it works?

Correct, we are still working on the tests, I've just submitted it for an early review by the vega team.

@voltaireChocolatine voltaireChocolatine force-pushed the feature/locked-sell branch 2 times, most recently from 5db461b to 5ae2e12 Compare October 9, 2024 18:21
@voltaireChocolatine
Copy link
Contributor Author

@candida-d I've merged all your changes.
@wwestgarth I've added tests for the feature.

Signed-off-by: Voltaire Chocolatine <q7mbqtQKGpsfaPAHKpHJ5Dgdg6at3YBQzmR@proton.me>
@jeremyletang jeremyletang merged commit 260cc2e into vegaprotocol:develop Oct 17, 2024
12 of 16 checks passed
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.

6 participants