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

setup pyth to use imported pyth package #71

Closed
wants to merge 6 commits into from
Closed

setup pyth to use imported pyth package #71

wants to merge 6 commits into from

Conversation

noisekit
Copy link
Contributor

@noisekit noisekit commented Nov 13, 2023

  • Add pyth configs but as a separate PR
  • TODO solve issues with simulate release tests.

Extracted from #66 as it affects other deployments

cc @dbeal-eth

@noisekit noisekit requested a review from dbeal-eth November 13, 2023 05:36
@noisekit noisekit self-assigned this Nov 13, 2023
Base automatically changed from base-goerli-andromeda to main November 13, 2023 06:01
Copy link
Contributor Author

@noisekit noisekit left a comment

Choose a reason for hiding this comment

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

We must not change settlement strategy settings.

@@ -3,7 +3,7 @@
[setting.pythFeedUrl]

[setting.settlementReward]
defaultValue = "0"
defaultValue = "0.1"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because settlement settings are not updatable but append only we cannot just change settings. May I suggest removing this file entirely and inline settings into the addSettlementStrategy invoke instead? Because once we add it we cannot update or remove it. Any change will require adding a new strategy and disabling the old one.

@@ -49,7 +49,7 @@ func = "addSettlementStrategy"
args = [
"<%= settings.btcPerpsMarketId %>",
# strategyType = 0 (pyth)
{ strategyType = "0", settlementDelay = "<%= settings.bigCapSettlementDelay %>", settlementWindowDuration = "<%= settings.bigCapSettlementWindowDuration %>", priceWindowDuration = "<%= settings.bigCapPriceWindowDuration %>", priceVerificationContract = "<%= settings.pythPriceVerificationAddress %>", feedId = "<%= settings.pythBtcFeedId %>", url = "<%= settings.pythFeedUrl %>", settlementReward = "<%= settings.settlementReward %>", priceDeviationTolerance = "<%= parseEther('1') %>", disabled = false }
{ strategyType = "0", settlementDelay = "<%= settings.bigCapSettlementDelay %>", settlementWindowDuration = "<%= settings.bigCapSettlementWindowDuration %>", priceWindowDuration = "<%= settings.bigCapPriceWindowDuration %>", priceVerificationContract = "<%= settings.pythPriceVerificationAddress %>", feedId = "<%= settings.pythBtcFeedId %>", url = "<%= settings.pythFeedUrl %>", settlementReward = "<%= parseEther(settings.settlementReward) %>", priceDeviationTolerance = "<%= parseEther('1') %>", disabled = false }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suggest not to wrap settings into parseEther to avoid confusion. And instead use parseEther in settings itself defaultValue = "<%= parseEthers() %>"
This change introduces variability in how settings are used as some of them are already in wei and some are plain numbers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have somewhat related idea (not yet formed) on how to use something like zod schema to validate values, so they do come in at least expected ranges.
This is not part of generic cannon setup but perhaps can be implemented as a synthetix-specific plugin? So we declare, let's say, that settlementReward is expected to be in wei, and if we detect values that are not in a reasonable wei range - validation fails.
This schema can potentially be half-auto extracted from solidity. And/or at least be collocated with contract itself.
Would appreciate any feedback on the idea so we can at least avoid wildly incorrect valued sent to our contracts by mistake

@noisekit noisekit closed this Nov 24, 2023
@noisekit noisekit deleted the pyth-pack branch November 24, 2023 21:44
@noisekit
Copy link
Contributor Author

This is a part of #73

@noisekit noisekit mentioned this pull request Nov 24, 2023
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.

4 participants