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

Create a proposal for a execution emitters coins split #1706

Closed
wants to merge 2 commits into from

Conversation

krhubert
Copy link
Contributor

@krhubert krhubert commented Mar 5, 2020

dep #1703

@krhubert krhubert added the enhancement New feature or request label Mar 5, 2020
@krhubert krhubert added this to the next milestone Mar 5, 2020
@krhubert krhubert requested a review from NicolasMahe March 5, 2020 21:16
@krhubert krhubert self-assigned this Mar 5, 2020
@krhubert krhubert force-pushed the feature/execution-emitters-paymant branch from 4b552a8 to e530135 Compare March 6, 2020 09:36
@krhubert krhubert force-pushed the feature/execution-emitters-paymant branch from e530135 to 0f8ae34 Compare March 6, 2020 09:38
var runnerShare = sdk.NewDecWithPrec(9, 1)
var (
runnerShare = sdk.NewDecWithPrec(8, 1)
emittersShare = sdk.NewDecWithPrec(1, 1)
Copy link
Member

Choose a reason for hiding this comment

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

just for comprehension, it would be nice to also have the serviceShare and assert in the code if the rest of runner + emitters != serviceShare. Like that, we will have 100%

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes and no. We need to be sure that inputs == outputs and we can only have that, if we subtract it, because division can't guarantee that.

Copy link
Member

@antho1404 antho1404 Mar 7, 2020

Choose a reason for hiding this comment

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

of course but we can still check that the diff matches the supposed value of the service. Anyway not blocking but it's just really confusing not to have all the repartition here, it looks like there is only 90% of the distribution and the rest maybe stays in the execution. (If we have a deep look at the code then we see it's not the case but we should not need to have a deep look at the code). Maybe just document that correctly with the details of the repartitions.


var voters []sdk.AccAddress
var emitterCoins sdk.Coins
if store.Has(voteKey(execHash)) {
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't do this test, if there is no vote on the store it means we have a serious issue before and we should either panic or at least return an error and not just ignore it I think. Or we could calculate everything and at the end assert that the result of emitters + runner + service == balance of execution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is related to a question if we should put a exec creator as an voter into the list. If yes, then this check shouldn't be here, otherwise it's should be :)

}

// if there is no emitter get rest coins to runner
if emitterCoins.IsZero() {
Copy link
Member

Choose a reason for hiding this comment

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

why should we do that? why would we have no emitters in the first place? and why giving to the runner and not the service or get back to the user etc... we never talked about that...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why would we have no emitters in the first place?

See above. Also, it might happen that coins/len(emitters) == 0, so we can either pay only a few random emitters or none at all.

and why giving to the runner

Because I took the reward from runners' share.

Copy link
Member

Choose a reason for hiding this comment

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

Because I took the reward from runners' share.

No, you're giving to the runner from the execution here... or I think there is a big confusion with what is the runner and all the different types of runners (emitters/executors).
The different types of runners will do a specific job and will be rewarded for that and not for the job of others.

Also, I'm seeing that in fact, this instruction is only when the coins are zero and you add zero to the existing coins. So yes we will actually not pay anyone and this whole instruction is useless isn't it?

@NicolasMahe
Copy link
Member

closing this PR in favor of #1705

@NicolasMahe NicolasMahe deleted the feature/execution-emitters-paymant branch March 12, 2020 11:54
@NicolasMahe NicolasMahe removed this from the v0.20 milestone Mar 16, 2020
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

Successfully merging this pull request may close these issues.

3 participants