Skip to content

Erdpy network providers integration in erdpy#172

Merged
popenta merged 33 commits into
feat/v4from
erdpy-eggs-reference
Jan 6, 2023
Merged

Erdpy network providers integration in erdpy#172
popenta merged 33 commits into
feat/v4from
erdpy-eggs-reference

Conversation

@popenta
Copy link
Copy Markdown
Collaborator

@popenta popenta commented Dec 28, 2022

Integrated erdpy-network-providers in erdpy.

The purpose of this pr is to replace the old ProxyNetworkProviders that erdpy used with the ones from erdpy-network-providers.

Added unit tests.

@popenta popenta self-assigned this Dec 28, 2022
@andreibancioiu andreibancioiu self-requested a review January 5, 2023 11:47
claudiu725
claudiu725 previously approved these changes Jan 5, 2023
Copy link
Copy Markdown
Contributor

@andreibancioiu andreibancioiu left a comment

Choose a reason for hiding this comment

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

👍

Comment thread setup.py Outdated
Comment thread erdpy/cli_block.py
Comment thread erdpy/contracts.py Outdated
from erdpy.interfaces import IElrondProxy
from erdpy.transactions import Transaction
from erdpy.utils import Object
from erdpy_network_providers.proxy_network_provider import ContractQuery
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ideally, we should not import this, but define a similar class (that implements the IContractQuery interface) here in erdpy. Can stay as it is for the moment, though.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Imported the IContractQuery interface and implemented a new ContractQuery class similar to the one in erdpy-network-providers.

Comment thread erdpy/simulation.py
def simulate_transaction(self, transaction: Dict[str, Any]) -> ISimulateResponse:
...

def simulate_transaction_cost(self, transaction: Dict[str, Any]) -> ISimulateCostResponse:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe drop this?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes, this will be dropped. If i remember correctly we've discussed to remove simulate_transaction_cost in v4.0.0, but I was thinking to do it in a separate PR. What do you think?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

OK, can be done in a separate PR 👍

Comment thread erdpy/simulation.py

class Simulator():
def __init__(self, proxy: IElrondProxy) -> None:
def __init__(self, proxy: INetworkProvider) -> None:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe drop everything related to simulate_transaction_cost?

If I remember well, this function is not implemented in eggs-network-providers.

Comment thread erdpy/tests/test_accounts_repository.py Outdated


class INetworkProvider(Protocol):
def get_account_nonce(self, address: IAddress) -> int:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This isn't implemented by erdpy-network-providers, if I remember well. Remove all calls to it (in erdpy)?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

And delete the ElrondProxyStub class as well?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think the stub has to stay, but to implement get_account(), instead?

repository.sync_nonces(proxy)

I think this function calls get_account().nonce, right? get_account_nonce() should normally not exist anymore in the code.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh, I see you already did that.

Comment thread erdpy/tests/test_proxy.py
@@ -0,0 +1,140 @@
from pathlib import Path
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍 for having this kind of tests.

Comment thread erdpy/transactions.py
self.hash = txOnNetwork.hash
return txOnNetwork

def __send_transaction_and_wait_for_result(self, proxy: INetworkProvider, payload: Any, num_seconds_timeout: int = 100) -> ITransactionOnNetwork:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

All right, until we figure out a better place for this logic.

@popenta popenta merged commit 4a8e4e0 into feat/v4 Jan 6, 2023
@popenta popenta deleted the erdpy-eggs-reference branch January 6, 2023 10:40
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