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

SIP-406 Add trading enabled feature flag #2339

Merged
merged 20 commits into from
Dec 12, 2024
Merged

SIP-406 Add trading enabled feature flag #2339

merged 20 commits into from
Dec 12, 2024

Conversation

0xMithrandir
Copy link
Contributor

@0xMithrandir 0xMithrandir commented Nov 21, 2024

Adds a feature flag tradingEnabled which has to be enabled to be trading/wrapping synths.

https://sips.synthetix.io/sips/sip-406/

@0xMithrandir 0xMithrandir marked this pull request as ready for review November 28, 2024 07:53
Copy link
Contributor

@dbeal-eth dbeal-eth left a comment

Choose a reason for hiding this comment

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

please see comments

while we are at it, do we also have a feature flag to turn on/off all markets?

and do we have a feature flag to only turn off withdrawals/deposits? I feel like this is a level of granularity we might actuall ywant to have, but I understand we probably don't want to expand the scope.

@@ -31,6 +34,11 @@ contract AtomicOrderModule is IAtomicOrderModule {
SpotMarketFactory.Data storage spotMarketFactory = SpotMarketFactory.load();
spotMarketFactory.validateMarket(marketId);

ITokenModule synth = SynthUtil.getToken(marketId);
FeatureFlag.ensureAccessToFeature(
bytes32(abi.encodePacked(Flags.TRADING_ENABLED, marketId))
Copy link
Contributor

Choose a reason for hiding this comment

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

note: while this works fine for progmatic use cases, afaict its not like the human readable text of the flag would be:

tradingEnabledSynthId75

if the pool ID was 75. we would need to first encode the integer as a string i am pretty sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, it's hard to decipher which FeatureUnavailable is thrown with this setup.

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've updated this line to encode the marketId to string as well.

@0xMithrandir
Copy link
Contributor Author

please see comments

while we are at it, do we also have a feature flag to turn on/off all markets?

and do we have a feature flag to only turn off withdrawals/deposits? I feel like this is a level of granularity we might actuall ywant to have, but I understand we probably don't want to expand the scope.

This SIP is quite specific so I'd like to avoid expanding the scope but maybe @kaleb-keny can chime in here when he gets back on adding more flags.

@kaleb-keny
Copy link
Member

please see comments
while we are at it, do we also have a feature flag to turn on/off all markets?
and do we have a feature flag to only turn off withdrawals/deposits? I feel like this is a level of granularity we might actuall ywant to have, but I understand we probably don't want to expand the scope.

This SIP is quite specific so I'd like to avoid expanding the scope but maybe @kaleb-keny can chime in here when he gets back on adding more flags.

hey @0xMithrandir i am happy to alter the sip, i think it makes sense to add a master control for killing the entire spot market...
For deposit withdrawal, not sure what we are depositing withdrawing, since this is more of an exchange part of the code base... I think this is possible to in asyncOrders with that isEnabled flag on the settle strategy... But not possible with atomic orders and wrappers, hence this sip...

So it would be more like a atomicOrderFeatureFlag=1/0 and wrapperFlag=1/0
Let me know of your thoughts and happy to make the necessary changes to the sip

@0xMithrandir
Copy link
Contributor Author

please see comments
while we are at it, do we also have a feature flag to turn on/off all markets?
and do we have a feature flag to only turn off withdrawals/deposits? I feel like this is a level of granularity we might actuall ywant to have, but I understand we probably don't want to expand the scope.

This SIP is quite specific so I'd like to avoid expanding the scope but maybe @kaleb-keny can chime in here when he gets back on adding more flags.

hey @0xMithrandir i am happy to alter the sip, i think it makes sense to add a master control for killing the entire spot market... For deposit withdrawal, not sure what we are depositing withdrawing, since this is more of an exchange part of the code base... I think this is possible to in asyncOrders with that isEnabled flag on the settle strategy... But not possible with atomic orders and wrappers, hence this sip...

So it would be more like a atomicOrderFeatureFlag=1/0 and wrapperFlag=1/0 Let me know of your thoughts and happy to make the necessary changes to the sip

You'd prefer having separate flags for each module like:

  • atomicOrdersEnabled per synth
  • wrapperEnabled per synth
  • general tradingEnabled flag that is for all markets?

@kaleb-keny
Copy link
Member

replied in discord, but the context is

  • spotMarketEnabled would enable the entire spot market for all synths, i.e. atomic, async and wrappers
  • atomicOrdersEnabled would enable atomic orders on a specific synth, where by the non sUSD need to be enabled for the transaction to go through
  • wrapperEnabled would enable wraps & unwraps on a specific synth, where by the non sUSD need to be enabled for the transaction to go through
    As for async orders per synth they can be enabled / disabled with the settle strategy flag. So no changes required on those
    I will update the sip accordingly

@0xMithrandir
Copy link
Contributor Author

I've updated the new feature flags, I'll add more specific tests but lmk if this is looking good now @dbeal-eth

@dbeal-eth
Copy link
Contributor

dbeal-eth commented Dec 4, 2024

@0xMithrandir lgtm! do we have a audit completed for this change? normally we wait for audit before merging.

@0xMithrandir
Copy link
Contributor Author

@0xMithrandir lgtm! do we have a audit completed for this change? normally we wait for audit before merging.

I'll submit it today for audit, after their approval it can be merged.

@0xMithrandir 0xMithrandir merged commit 0e431c3 into main Dec 12, 2024
27 checks passed
@0xMithrandir 0xMithrandir deleted the sip-406 branch December 12, 2024 13:12
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.

3 participants