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

Gas usage optimization #480

Merged
merged 2 commits into from May 9, 2022

Conversation

mr-kenikh
Copy link
Contributor

I would like to propose a change that might slightly optimize gas usage by reducing the number of messages in a transaction. It is not necessary to collect the rewards first and then make the delegation, it is just enough to delegate.

Pros: reduced costs for validators who serve a large number of delegators.
Cons: the transaction becomes less informative. It may not be clear from which funds the delegation comes. Often explorers have an "auto claim rewards" field, which partially resolves this problem.

Examples:

  1. Get reward + delegate (current implementation)
  2. Delegate only (current propose)

@tombeynon
Copy link
Contributor

This is very interesting! I'm just about to raise a separate issue to reduce the gas estimate multiplier that's used right now as that's over-zealous.

Your PR would obviously bring it down even further but I didn't expect this to work for users without a balance. Have you tested that side of things?

We also have to be very careful not to touch the user's existing balance and only delegate their rewards, and this implementation would risk race conditions that would cause us to delegate from balance, e.g. if user delegated while the script was running. It would be a very nice change if we can make it work though

@mr-kenikh
Copy link
Contributor Author

What do you mean by "for users without a balance"? We don't touch user's balance, because we pay fees.
Current propose is the same as current implementation, but the only difference is that the rewards are claimed automatically.

  1. Firstly, we fetch total rewards here:

    const totalRewards = await this.totalRewards(client, address)

  2. And then use this value here:

    amount: coin(amount, denom)

  3. Thus, there is no way to touch user's balance. Moreover, user's balance will increase as you described in this issue (btw, that issue can't be resolved).

@tombeynon
Copy link
Contributor

tombeynon commented May 4, 2022

The result of this TX would mean the user had a non-zero balance, but if they had e.g 0.001 balance and 0.1 of rewards, I'm pretty sure the TX would fail with your change. They don't have the balance to delegate until after the TX succeeds.

Very happy to be proven wrong though. We could still take advantage of this since we know the users balance beforehand and could only include the Withdraw if necessary.

@mr-kenikh
Copy link
Contributor Author

https://explorer.yummy.capital/txs/31DEC0F17AF797793D016436A5096E1D58FE58F3DF8061B18D4D716167F3BE79

Target address: cro1rkvlrss04eay5mll79nclr3nc34rq9qnedc6k0
a) balance before: 1.00809988
b) restaked amount: 1.01416654
c) auto claim rewards: 1.01441617
d) balance after: 1.00834951

  1. b > a, what was required for testing
  2. с > b, as expected
  3. a + (c - b) = d, as expected
  4. transaction succeeded, as expected 😄

@tombeynon
Copy link
Contributor

Wow.. I am very glad to be proven wrong. This is awesome and works great in my testing! I'm sure I tried this originally...

I'm just preparing a second PR which will also remove all of the Withdraw grant functionality from the UI etc too. We can simplify quite a few things with this change. Will look to release later today.

FYI this saves approx another 25% of the total gas fee which is brilliant. Combined with #481 we should be in the region of 50% gas saved overall.

@tombeynon tombeynon mentioned this pull request May 5, 2022
@tombeynon tombeynon merged commit ca7cdff into eco-stake:master May 9, 2022
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.

None yet

2 participants