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

add mainnet examples, readme and configs #97

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

kaloureyes3
Copy link

No description provided.

@kaloureyes3
Copy link
Author

there is an issue in post.py file because the networkConfig needs to be passed pointing either to testnet or mainnet again, where it should be done only once in the main python file to be executed
it will be updated soon

@johnqh johnqh self-requested a review January 10, 2024 20:10
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.

In addition to the comments in code,

"price": 40000,
"size": 0.01,
"client_id": randrange(0, MAX_CLIENT_ID),
"good_till_block": client.get_current_block() + 11,
Copy link
Contributor

Choose a reason for hiding this comment

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

Please change to good_til_block

price=order["price"],
size=order["size"],
client_id=order["client_id"],
good_til_block=order["good_till_block"],
Copy link
Contributor

Choose a reason for hiding this comment

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

Please do a global search and replace from good_till_block to good_til_block

"""Network configurations."""

import warnings
from dataclasses import dataclass
from typing import Optional, Union

from v4_client_py.clients.constants import (
Copy link
Contributor

Choose a reason for hiding this comment

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

We do not handle main net deployment. Thus, there should not be any reference to mainNet in the code base. Also, consumer of the library should have the freedom to pick the validator and indexer endpoints to connect to.

My suggestion, is to add this convenient function to Network class

    @classmethod
    def config_network(
        grpc_endpoint: str,
        rest_endpoint: str,
        websocket_endpoint: str,
        chain_id: str,
    ) -> Network:
        validator_config=ValidatorConfig(
            grpc_endpoint=grpc_endpoint,
            chain_id=chain_id,
            ssl_enabled=True
        )
        indexer_config=IndexerConfig(
            rest_endpoint=rest_endpoint,
            websocket_endpoint=websocket_endpoint,
        )
        return Network(
            env='custom',
            validator_config=validator_config,
            indexer_config=indexer_config,
        )

Then in documentation (or in mainnet example file), replace
network = Network.testnet()
With

network = Network.config_network(
        grpc_endpoint = '{get from deployer}',
        rest_endpoint= '{get from deployer}',
        websocket_endpoint = '{get from deployer}',
        chain_id = '{get from deployer}',
    )

@@ -0,0 +1,20 @@
# User guide to test mainnet examples

Copy link
Contributor

Choose a reason for hiding this comment

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

We need an #integration section in README.md, probably under #installation, with the simple instructions on how to use the code.

Essentially it should cover two things to get it started

  1. Set up network configuration
network = Network.config_network(
        grpc_endpoint = '{get from deployer}',
        rest_endpoint= '{get from deployer}',
        websocket_endpoint = '{get from deployer}',
        chain_id = '{get from deployer}',
    )
  1. Connect to client
    client = CompositeClient(
        network,
    )

Copy link
Author

Choose a reason for hiding this comment

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

we still have the problem of the config.py for the NetworkConfig class that needs to be defined

@kaloureyes3
Copy link
Author

kaloureyes3 commented Jan 18, 2024

hi @johnqh
i have added in ~/.../v4-clients/v4-client-py/v4_client_py/clients/constants.py the configs for the networks, so the user just need to go to this file, place constants here (whether mainnet or testnet) and use the SDK
thought this is more simple than having the configs in each single example file
added to instructions as well under examples/mainnet_examples/README.md

AERIAL_GRPC_OR_REST_PREFIX = '{get from deployer}'
INDEXER_REST_ENDPOINT = '{get from deployer}'
INDEXER_WS_ENDPOINT = '{get from deployer}'
CHAIN_ID = '{get from deployer}'
Copy link
Contributor

Choose a reason for hiding this comment

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

We still have miscommunications.

Those shouldn't be in the library const. A library should be used by installing with pypi. When you have consts which requires modifications, the consumer of this library will have to download the source code of the library, change these consts, and integrate the library source code into their code directly instead of pypi.

dYdX Trading Inc runs a testNet so it is OK to have testnet configurations in the library. However, we are not deployer of mainNet so the library do not have reference to mainNet.

My previous comment is all about providing a clear pathway to use this library through pypi for deployer's mainNet.

Of course, if this library is distributed by dYdX Foundation, it is totally OK to have their mainnet configs here since they are the deployer. But dYdX Trading Inc is not the deployer and we can only support custom configurations.

ORDER_STATUS_FILLED = 'FILLED'
ORDER_STATUS_CANCELED = 'CANCELED'
ORDER_STATUS_UNTRIGGERED = 'UNTRIGGERED'
ORDER_STATUS_PENDING = "PENDING"
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that you have a custom lint configurations? Can you have the lint configuration somewhere? Different lint will just cause the source to switch between single quotes and double quotes between commits.

@@ -148,3 +103,18 @@ def latest_stable_testnet(cls) -> "NetworkConfig":
DeprecationWarning,
)
return cls.fetch_dydx_stable_testnet()

@classmethod
def fetchai_network_config(cls, chain_id: str, url_prefix: str, url: str) -> "NetworkConfig":
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not use FetchAI in our code. The original code (code in chain/aerial) was a folk from fetchai/cosmpy and their low level code has some references to fetchai (which we should remove), but our high level code should never refer fetchai. This only causes confusion.

@johnqh johnqh self-requested a review January 22, 2024 17:13
@johnqh
Copy link
Contributor

johnqh commented Jan 22, 2024

Is the other PR from your company? #106

That PR handles the network configs well. If you roll this branch back to the commit point of previous PR, incorporate the network config files from the other PR, then it is almost there. I think we only needed change from "good_till_time" to "good_til_time" and some readme comments to be good.

@dydxwill
Copy link
Contributor

dydxwill commented Jul 5, 2024

Please address @johnqh's comments and I can approve.



# ------------ API URLs ------------
INDEXER_API_HOST_MAINNET = None
Copy link

Choose a reason for hiding this comment

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

it would be great if there is one example for mainnet, because examples work great on testnet but then all kind of DNS issues come up for mainnet

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.

4 participants