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

feat/docs: CL external incentives tool #5458

Merged
merged 3 commits into from
Jun 10, 2023
Merged

feat/docs: CL external incentives tool #5458

merged 3 commits into from
Jun 10, 2023

Conversation

p0mvn
Copy link
Member

@p0mvn p0mvn commented Jun 7, 2023

Closes: #XXX

What is the purpose of the change

This PR modifies the script to add the ability to create external incentives via gauges and wait for an epoch so that it is converted into an incentive record.

Testing and Verifying

This change is a trivial rework / code cleanup without any test coverage.

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes?
  • Changelog entry added to Unreleased section of CHANGELOG.md?

Where is the change documented?

  • Specification (x/{module}/README.md)
  • Osmosis documentation site
  • Code comments?
  • N/A

@p0mvn p0mvn added V:state/compatible/no_backport State machine compatible PR, depends on prior breaks A:no-changelog labels Jun 7, 2023
@p0mvn p0mvn marked this pull request as ready for review June 7, 2023 19:51
@p0mvn p0mvn requested a review from a team as a code owner June 7, 2023 19:51
@github-actions github-actions bot added C:docs Improvements or additions to documentation T:build labels Jun 7, 2023
log.Fatal(err)
}

log.Println("incentive records. If empty, something probably went wrong", incentiveRecordsResponse.IncentiveRecords)
Copy link
Contributor

@stackman27 stackman27 Jun 7, 2023

Choose a reason for hiding this comment

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

can we also check gauge Records after we distribute? FilledEpoch should be +1 and distribtued Coins should be appended to the gauge struct

Copy link
Contributor

Choose a reason for hiding this comment

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

also should we consider all the created incentives regardless of poolId <> gaugeId association and distribute tokens to them as well?

Copy link
Member Author

@p0mvn p0mvn Jun 8, 2023

Choose a reason for hiding this comment

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

The purpose of this is to make sure that incentive records are created from gauges. This print gets the job done.

I don't think what you're requesting is helpful for this tool. However, it would be a good assertion to make in tests

Copy link
Member

@mattverse mattverse left a comment

Choose a reason for hiding this comment

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

utACK.

A little concerned about the

log.Println("current epoch after gauge creation", currentEpochResponse.CurrentEpoch)
log.Println("waiting for next epoch...")

getting logged for an hour, otherwise lgtm

tests/cl-go-client/main.go Outdated Show resolved Hide resolved
tests/cl-go-client/main.go Show resolved Hide resolved
tests/cl-go-client/main.go Show resolved Hide resolved
tests/cl-go-client/main.go Outdated Show resolved Hide resolved
Comment on lines +285 to +286
log.Println("current epoch after gauge creation", currentEpochResponse.CurrentEpoch)
log.Println("waiting for next epoch...")
Copy link
Member

Choose a reason for hiding this comment

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

does this get printed for an hour?

Copy link
Member

Choose a reason for hiding this comment

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

maybe we can set a timer and retry every minute

Copy link
Member Author

Choose a reason for hiding this comment

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

On localosmosis, unintuitively, an "hour" epoch is actually 60 seconds so this shouldn't be a concern.

If this is used on testnet, there is no "hour" epoch so the script would fail early.

Thi script is mainly used for testing incentives on localosmosis. If there is an infinite loop somewhere, the user can just kill the script

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see, now I understand why everything was so confusing while reviewing this, now everything became more clearer

p0mvn and others added 2 commits June 8, 2023 19:25
Comment on lines +62 to +63
// Note, this is localosmosis-specific.
expectedEpochIdentifier = "hour"
Copy link
Member

Choose a reason for hiding this comment

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

Are we going to run into a problem trying to use this for cl-testnet?

Copy link
Member Author

Choose a reason for hiding this comment

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

The epoch identifier can manually be changed to "day" here.

A bigger problem is that we would have to wait for the epoch to pass for a while so that the incentive records are created

Copy link
Member Author

Choose a reason for hiding this comment

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

I will try running this against testnet before I merge and document all the changes that would be necessary

@p0mvn
Copy link
Member Author

p0mvn commented Jun 10, 2023

OK, I'm going to merge this script to unblock the external incentives PR branched off of this. I asked Ibrar to test this out to make sure that all needed functionality is present

@p0mvn p0mvn merged commit b030ab1 into main Jun 10, 2023
@p0mvn p0mvn deleted the roman/cl-incentive branch June 10, 2023 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:no-changelog C:docs Improvements or additions to documentation T:build V:state/compatible/no_backport State machine compatible PR, depends on prior breaks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants