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.

Sign up for GitHub

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

Jump to bottom

Add extrinsics for editing markets #834

Merged
merged 12 commits into from
Oct 31, 2022
Merged

Add extrinsics for editing markets #834

merged 12 commits into from
Oct 31, 2022

Conversation

vivekvpandya
Copy link
Contributor

This PR should close #742

@vivekvpandya vivekvpandya added the s:in-progress The pull requests is currently being worked on label Oct 13, 2022
@vivekvpandya vivekvpandya added this to the v0.3.7 milestone Oct 13, 2022
@vivekvpandya vivekvpandya self-assigned this Oct 13, 2022
@maltekliemann maltekliemann mentioned this pull request Oct 13, 2022
Closed
@vivekvpandya vivekvpandya added s:review-needed The pull request requires reviews and removed s:in-progress The pull requests is currently being worked on labels Oct 14, 2022
@maltekliemann
Copy link
Member

Starting my review now...

maltekliemann
maltekliemann requested changes Oct 18, 2022
View reviewed changes
Copy link
Member

@maltekliemann maltekliemann left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Nice, I like the thorough testing. Some things, though:

  • The changelog for developers needs to be updated.
  • I think you should also test that request_edit fails if the market is not Proposed (I may have missed that)

Missing Features

Advisors should be able to edit markets, as well. However, I think this warrants further discussion with the committee. I'll let you know about what was decided tomorrow.

Concerns

I'm also a bit worried about letting people change the market period. This creates an exploit for getting around slashing: Let's say I create a market which I don't know how to fix, I get asked by the committee to edit and so I set the market period to something like current_block..next_block. And then the market expires and I get my deposit back. I admit this is a very contrived example. But still...

I guess one solution is to release the advisory bond when an edit request is made, but that opens up a whole new can of worms. The alternative is to slash the advisory bond of expired markets. Seems fair to me. I'll bring this up at the Advisory Committee meeting tomorrow and let you know what the others think.

zrml/prediction-markets/src/benchmarks.rs Outdated Show resolved Hide resolved
zrml/prediction-markets/src/lib.rs Outdated Show resolved Hide resolved
zrml/prediction-markets/src/lib.rs Outdated Show resolved Hide resolved
zrml/prediction-markets/src/lib.rs Show resolved Hide resolved
zrml/prediction-markets/src/benchmarks.rs Outdated Show resolved Hide resolved
zrml/prediction-markets/src/benchmarks.rs Outdated Show resolved Hide resolved
zrml/prediction-markets/src/lib.rs Outdated Show resolved Hide resolved
zrml/prediction-markets/src/lib.rs Outdated Show resolved Hide resolved
zrml/prediction-markets/src/benchmarks.rs Outdated Show resolved Hide resolved
zrml/prediction-markets/src/lib.rs Outdated Show resolved Hide resolved
@Chralt98 Chralt98 marked this pull request as ready for review October 19, 2022 07:05
@Chralt98
Copy link
Member

I wait for the apply of the review suggestion from Malte.

@maltekliemann maltekliemann added i:spec-changed ⚠️ Implies change in spec version i:transactions-changed ⚠️ Implies change in transaction version labels Oct 21, 2022
maltekliemann
maltekliemann requested changes Oct 22, 2022
View reviewed changes
zrml/prediction-markets/src/lib.rs Outdated Show resolved Hide resolved
zrml/prediction-markets/src/lib.rs Outdated Show resolved Hide resolved
zrml/prediction-markets/src/lib.rs Outdated Show resolved Hide resolved
zrml/prediction-markets/src/lib.rs Outdated Show resolved Hide resolved
zrml/prediction-markets/src/lib.rs Outdated Show resolved Hide resolved
@maltekliemann maltekliemann added s:revision-needed The pull requests must be revised and removed s:review-needed The pull request requires reviews labels Oct 22, 2022
@Chralt98
Copy link
Member

Starting my review now..

Chralt98
Chralt98 reviewed Oct 24, 2022
View reviewed changes
Copy link
Member

@Chralt98 Chralt98 left a comment

Choose a reason for hiding this comment

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

Cool, Vivek, with your help, the markets can now be edited in the Proposed state. As long as the markets are in the proposed state I feel confident, that this enhancement is safe.

Apart from Malte's suggestion, I've some smaller fixes. PS: And I love the tests too!

zrml/prediction-markets/src/lib.rs Show resolved Hide resolved
zrml/prediction-markets/src/lib.rs Outdated Show resolved Hide resolved
zrml/prediction-markets/src/lib.rs Outdated Show resolved Hide resolved
zrml/prediction-markets/src/lib.rs Outdated Show resolved Hide resolved
zrml/prediction-markets/src/lib.rs Outdated Show resolved Hide resolved
zrml/prediction-markets/src/lib.rs Outdated Show resolved Hide resolved
zrml/prediction-markets/src/lib.rs Outdated Show resolved Hide resolved
zrml/prediction-markets/src/lib.rs Show resolved Hide resolved
zrml/prediction-markets/src/tests.rs Show resolved Hide resolved
zrml/prediction-markets/src/tests.rs Show resolved Hide resolved
@vivekvpandya vivekvpandya added s:review-needed The pull request requires reviews and removed s:revision-needed The pull requests must be revised labels Oct 25, 2022
Chralt98
Chralt98 reviewed Oct 25, 2022
View reviewed changes
Copy link
Member

@Chralt98 Chralt98 left a comment

Choose a reason for hiding this comment

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

Just a quick explanation of the events and the extrinsics added to the changelog_for_devs file is missing.

Chralt98
Chralt98 previously approved these changes Oct 25, 2022
View reviewed changes
Copy link
Member

@Chralt98 Chralt98 left a comment

Choose a reason for hiding this comment

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

OK

@maltekliemann
Copy link
Member

Starting my review now...

maltekliemann
maltekliemann requested changes Oct 27, 2022
View reviewed changes
Copy link
Member

@maltekliemann maltekliemann left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I'm also having trouble finding a test for the scenario where someone other than the market creator attempts to edit a market. Nevermind, found it.

runtime/common/src/lib.rs Outdated Show resolved Hide resolved
zrml/prediction-markets/src/lib.rs Outdated Show resolved Hide resolved
zrml/prediction-markets/src/lib.rs Outdated Show resolved Hide resolved
zrml/prediction-markets/src/lib.rs Outdated Show resolved Hide resolved
zrml/prediction-markets/src/lib.rs Outdated Show resolved Hide resolved
maltekliemann
maltekliemann previously approved these changes Oct 30, 2022
View reviewed changes
Copy link
Member

@maltekliemann maltekliemann left a comment

Choose a reason for hiding this comment

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

👍 Just one correction in the documentation, no need to do a re-review.

zrml/prediction-markets/src/lib.rs Outdated Show resolved Hide resolved
Chralt98
Chralt98 approved these changes Oct 31, 2022
View reviewed changes
Copy link
Member

@Chralt98 Chralt98 left a comment

Choose a reason for hiding this comment

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

OK

@vivekvpandya vivekvpandya added s:accepted This pull request is ready for merge and removed s:review-needed The pull request requires reviews labels Oct 31, 2022
@vivekvpandya vivekvpandya merged commit 01f4fbf into main Oct 31, 2022
@vivekvpandya vivekvpandya deleted the issue-742 branch October 31, 2022 09:04
@maltekliemann maltekliemann mentioned this pull request Mar 31, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Chralt98 Chralt98 Chralt98 approved these changes

@sea212 sea212 Awaiting requested review from sea212 sea212 is a code owner

@maltekliemann maltekliemann Awaiting requested review from maltekliemann maltekliemann is a code owner

Assignees

@vivekvpandya vivekvpandya

Labels
i:spec-changed ⚠️ Implies change in spec version i:transactions-changed ⚠️ Implies change in transaction version s:accepted This pull request is ready for merge
Projects
None yet
Milestone
v0.3.7
Development

Successfully merging this pull request may close these issues.

[prediction-markets] Allow metadata changes in pending advised markets
3 participants
@vivekvpandya @maltekliemann @Chralt98