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

update fee denom, url, use dydx testnet network config in posting txns #65

Merged
merged 9 commits into from
Oct 20, 2023

Conversation

dydxwill
Copy link
Contributor

No description provided.

@@ -81,7 +81,7 @@ def fetch_dydx_testnet(cls) -> "NetworkConfig":
chain_id="dydx",
url="grpc+https://v4.testnet.dydx.exchange",
fee_minimum_gas_price=5000000000,
Copy link
Contributor

Choose a reason for hiding this comment

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

I just noticed that we have this config as well. Can you update https://github.com/dydxprotocol/v4-clients/blob/main/v4-client-py/v4_client_py/chain/aerial/client/utils.py#L21 so that the fee param is Optional and if it's None, to use this value instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@johnqh
Copy link
Contributor

johnqh commented Oct 19, 2023

Also need to update version

@dydxwill
Copy link
Contributor Author

Also need to update version

where's version?

@dydxwill
Copy link
Contributor Author

updated value to 25000000000 per @johnqh slack convo

@vincentwschau
Copy link
Contributor

@dydxwill It's here

@dydxwill dydxwill requested a review from johnqh October 20, 2023 15:45
@dydxwill dydxwill changed the title update fee denom to adv4tnt update fee denom, url, use dydx testnet network config in posting txns Oct 20, 2023
amount=5_000_000,
amount=5,
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason for this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no reverted change here

@dydxwill dydxwill merged commit cdef96c into main Oct 20, 2023
1 check passed
Copy link
Contributor

@johnqh johnqh left a comment

Choose a reason for hiding this comment

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

I would have passed it just to unblock Lawrence but we are too close to mainnet. I would rather it not working than having unexpected behavior for MM

@@ -81,7 +81,7 @@ def fetch_dydx_testnet(cls) -> "NetworkConfig":
chain_id="dydx",
url="grpc+https://v4.testnet.dydx.exchange",
fee_minimum_gas_price=5000000000,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use the same number was TS?

25000000000

fee_denomination="dv4tnt",
chain_id="dydx-testnet-4",
url="grpc+https://dydx-testnet-archive.allthatnode.com:9090",
fee_minimum_gas_price=4630550000000000,
Copy link
Contributor

@johnqh johnqh Oct 20, 2023

Choose a reason for hiding this comment

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

You are mixing gas price and gas, and then essentially pass in a fixed gas for every function.

The gas is a calculated number based on the complexity of the function. So a fixed number can work for one function but not another.

Another issue: it is not clear whether it is using uusdc or adv4tnt for gas. They have different exponents and the real difference is factor of 10^12. If MM uses this code on mainnet, they may end up a huge shock!

The proper gas string format should be "$gas$denom".

Given we are close to mainnet, and MM are using the Python client, this is too dangerous.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants