polkascan / py-substrate-interface

Python Substrate Interface
https://polkascan.github.io/py-substrate-interface/
Apache License 2.0
241 stars 115 forks source link

Failing tests? #378

Closed MementoRC closed 7 months ago

MementoRC commented 7 months ago

It seems that the latest release have tests related to Balances transfer failing (in addition to the mpz already reported)

============================================================================================================= short test summary info ==============================================================================================================
FAILED test/test_create_extrinsics.py::CreateExtrinsicsTestCase::test_create_extrinsic_bytes_signature - ValueError: Call function 'Balances.transfer' not found
FAILED test/test_create_extrinsics.py::CreateExtrinsicsTestCase::test_create_extrinsic_metadata_v14 - ValueError: Call function 'Balances.transfer' not found
FAILED test/test_create_extrinsics.py::CreateExtrinsicsTestCase::test_create_mortal_extrinsic - ValueError: Call function 'Balances.transfer' not found
FAILED test/test_create_extrinsics.py::CreateExtrinsicsTestCase::test_create_multisig_extrinsic - ValueError: Call function 'Balances.transfer' not found
FAILED test/test_create_extrinsics.py::CreateExtrinsicsTestCase::test_payment_info - ValueError: Call function 'Balances.transfer' not found
FAILED test/test_keypair.py::KeyPairTestCase::test_create_ecdsa_keypair_mnemonic - AttributeError: 'mpz' object has no attribute 'to_bytes'
FAILED test/test_keypair.py::KeyPairTestCase::test_create_ecdsa_keypair_uri - AttributeError: 'mpz' object has no attribute 'to_bytes'
FAILED test/test_keypair.py::KeyPairTestCase::test_sign_and_verify_ecdsa - AttributeError: 'mpz' object has no attribute 'to_bytes'
FAILED test/test_keypair.py::KeyPairTestCase::test_sign_and_verify_invalid_signature_ecdsa - AttributeError: 'mpz' object has no attribute 'to_bytes'
FAILED test/test_keypair.py::KeyPairTestCase::test_sign_and_verify_public_ethereum_address - AttributeError: 'mpz' object has no attribute 'to_bytes'
FAILED test/test_type_registry.py::AutodetectAddressTypeTestCase::test_eth_address - AttributeError: 'mpz' object has no attribute 'to_bytes'
==================================================================================================== 11 failed, 288 passed in 420.59s (0:07:00) ============================
arjanz commented 7 months ago

Yes that's because in latest runtimes the deprecated "transfer" call is finally removed and now only available as "transfer_allow_death".

So not really something to worry about, but I'll update the unit tests for next release, which was long overdue.

MementoRC commented 7 months ago

Cool. Do you suggest to update the tests with transfer_allow_death?

I am finalizing a conda-forge recipe to bring substrate-interface to conda environments. I typically prefer to make as little changes as required, however, i also prefer to run the project's test suite just for peace of mind. In this case, I need to create 2 patches that remove the failing tests, as well as the mpz fix I put together: https://github.com/polkascan/py-substrate-interface/pull/353

It would also be great if you wouldn't mind become a co-maintainer of the recipe, let me know

arjanz commented 7 months ago

Cool. Do you suggest to update the tests with transfer_allow_death?

It doesn't matter that much for the unit tests, but in real life usage I recommend transfer_keep_alive because that call contains checks to make sure the account doesn't get reaped if the remaining balance drops below the existential deposit.

BTW I already committed a patch fixing these tests last months, but didnt't release it yet.. will be included in next release: https://github.com/polkascan/py-substrate-interface/compare/v1.7.5...master

It would also be great if you wouldn't mind become a co-maintainer of the recipe, let me know

Sure! Although I haven't worked with conda much yet, this might be a good reason to do so :)

arjanz commented 7 months ago

as well as the mpz fix I put together: #353

For some reason this didn't catch my attention, I will have a look and make a new release to fix both issues

arjanz commented 7 months ago

I created a new release containing both fixes: https://github.com/polkascan/py-substrate-interface/releases/tag/v1.7.6

MementoRC commented 7 months ago

Great. You will need to post a message in the recipe's PR: https://github.com/conda-forge/staged-recipes/pull/25470 to indicate your approval as a maintainer. Note that I also created recipes for scalecodec py-sr25519-bindings, py-ed25519-zebra-bindings and py-bip39-bindings have already been merged and are live for conda environments - I will add you as a maintainer on those as well

I am learning conda myself and committed myself to do most of the maintenance on these recipes. The maintenance is also fairly well automated by conda-forge, so we may not have to do much

mohammadworks commented 7 months ago

I created a new release containing both fixes: https://github.com/polkascan/py-substrate-interface/releases/tag/v1.7.6

hey what happened? My code for DOT transfer gets this error: Call function 'Balances.transfer' not found

What should I do to fix this problem?

my code:

RPC = ""
private_key = ""
address = ""
to_address = ""

client = SubstrateInterface(RPC)
keypair = Keypair.create_from_private_key(
    private_key=private_key,
    ss58_address=address,
    crypto_type=KeypairType.ED25519
)
transaction = client.compose_call(
    call_module='Balances',
    call_function='transfer',
    call_params={
        'dest': to_address,
        'value': amount * Decimal('10_000_000_000')
    }
)
signed_transaction = client.create_signed_extrinsic(call=transaction, keypair=keypair)
transaction_result = client.submit_extrinsic(signed_transaction, wait_for_inclusion=True)
print(transaction_result)
arjanz commented 7 months ago

hey what happened? My code for DOT transfer gets this error: Call function 'Balances.transfer' not found

What should I do to fix this problem?

In recent runtime upgrade on Polkadot/Kusama the already deprecated method transfer has been permanently replaced with transfer_allow_death. However, in most cases you want checks that the account doesn't get accidentally reaped, so you probably want to replace the call with transfer_keep_alive.

More information about the change: https://dev.to/aaronbassett/deprecated-dispatchables-removed-from-polkadot-sdk-3hph

MementoRC commented 7 months ago

@arjanz Reminder to post a messageon these PRs:

MementoRC commented 7 months ago

@arjanz There is a failing test in the suite. I had seen it in the previous version and created a patch to remove it (running the test suite is not required for the recipe). It did not show-up in the latest version and I assumed you had seen/fixed it. Surprisingly the staged recipe passed, but now it is showing-up in the feedstock registration. I plan to patch the feedstock to remove just that failing test:

=================================== FAILURES ===================================
________________ SubscriptionsTestCase.test_query_subscription _________________

self = <test.test_subscriptions.SubscriptionsTestCase testMethod=test_query_subscription>

    def test_query_subscription(self):

        def subscription_handler(obj, update_nr, subscription_id):

            return {'update_nr': update_nr, 'subscription_id': subscription_id}

        result = self.substrate.query("System", "Events", [], subscription_handler=subscription_handler)

>       self.assertEqual(result['update_nr'], 0)
E       AssertionError: 1 != 0

test/test_subscriptions.py:39: AssertionError

Also, commenting here since it may not be an issue but a test setup that I missed

arjanz commented 7 months ago

I also experience this sometimes and I suspect it is a race condition in the subscription code. True async support is on its way in the next major release, so for now it can be ignored. Perhaps it's best to remove the test as it's not 100% reliable now.

mohammadworks commented 7 months ago

I created a new release containing both fixes: https://github.com/polkascan/py-substrate-interface/releases/tag/v1.7.6

hey what happened? My code for DOT transfer gets this error: Call function 'Balances.transfer' not found

What should I do to fix this problem?

my code:

RPC = ""
private_key = ""
address = ""
to_address = ""

client = SubstrateInterface(RPC)
keypair = Keypair.create_from_private_key(
    private_key=private_key,
    ss58_address=address,
    crypto_type=KeypairType.ED25519
)
transaction = client.compose_call(
    call_module='Balances',
    call_function='transfer',
    call_params={
        'dest': to_address,
        'value': amount * Decimal('10_000_000_000')
    }
)
signed_transaction = client.create_signed_extrinsic(call=transaction, keypair=keypair)
transaction_result = client.submit_extrinsic(signed_transaction, wait_for_inclusion=True)
print(transaction_result)

My problem has not been solved yet.

arjanz commented 7 months ago

@mohammadworks Did you read my reply earlier?

In recent runtime upgrade on Polkadot/Kusama the already deprecated method transfer has been permanently replaced with transfer_allow_death. However, in most cases you want checks that the account doesn't get accidentally reaped, so you probably want to replace the call with transfer_keep_alive.

More information about the change: https://dev.to/aaronbassett/deprecated-dispatchables-removed-from-polkadot-sdk-3hph

In other words, replace in your code:

transaction = client.compose_call(
    call_module='Balances',
    call_function='transfer_keep_alive',
    call_params={
        'dest': to_address,
        'value': amount * Decimal('10_000_000_000')
    }
)
mohammadworks commented 7 months ago

What happens if I use transfer_allow_death?

arjanz commented 7 months ago

What happens if I use transfer_allow_death?

If the sender’s account is below the existential deposit (e.g. 1 DOT on polkadot) as a result of the transfer, the account will be reaped and dust is lost.

Sometimes you want that this behaviour, but most of times you want protection against that by using transfer_keep_alive (i.e. call will fail if balance of sender would fall below existential deposit)

mohammadworks commented 7 months ago

In the code below, I completely Empty an address using transfer:

RPC = ""
private_key = ""
address = ""
to_address = ""
balance = ""

client = SubstrateInterface(RPC)
keypair = Keypair.create_from_private_key(
    private_key=private_key,
    ss58_address=address,
    crypto_type=KeypairType.ED25519
)
fee = client.get_payment_info(client.compose_call(
    call_module='Balances',
    call_function='transfer',
    call_params={
        'dest': to_address,
        'value': balance * 10_000_000_000
    }
), keypair=keypair)['partialFee']
# The transaction fee is deducted from the withdrawal amount
transaction = client.compose_call(
    call_module='Balances',
    call_function='transfer',
    call_params={
        'dest': to_address,
        'value': (balance * 10_000_000_000) - fee
    }
)
signed_transaction = client.create_signed_extrinsic(call=transaction, keypair=keypair)
transaction_result = client.submit_extrinsic(signed_transaction, wait_for_inclusion=True)
print(transaction_result)

But now using transfer_keep_alive no longer works.