Create (raw) Transaction builder class

[ixje]

This is a TODO for a project (see side bar)

As part of the Lightweight SDK API we would like to have a class that eases creation of (raw) transactions. A raw transaction can be understood as a serialised Transaction that can be send to the network via a RPC node or directly on the TCP network.

The available Transactions and their possible attributes are described in the documentation. There are 2 existing examples on how to manually build raw transactions in python here. From that code you can see there is enough room to simplify this for the end user. e.g. adding a description attribute to the transaction is currently done as follows

 contract_tx.Attributes.append(TransactionAttribute(usage=TransactionAttributeUsage.Description, data="My raw contract transaction description"))

a simplified interface could be

contract_tx.addDescription("My raw contract transaction description")

Some ideas:

• simplify adding TX attributes (see example above)
• simplify setting transaction destinations. Currently you have to create TransactionOutputs (ref) using script hashes and alike. Instead we could support an API like contract_tx.set_destination_addr("neo address")
• support loading raw TX's and inspection
• support TX validation e.g.
  • check that CertUrl, DescriptionUrl, Description, Remark series do not exceed 255 chars (see docs)
  • check that ContractHash, ECDH series, Hash series does not exceed 32 bytes (see docs)
  • check that there are no more than 16 attributes per TX (ref)
  • check that the maximum TX size is not exceeded (ref)

For the first iteration we should only support the following transactions

• ClaimTransaction - for claiming GAS
• ContractTransaction - for sending NEO/GAS
• InvocationTransaction - for interacting with smart contracts

[jseagrave21]

@ixje Could you take a look at https://github.com/jseagrave21/neo-python/blob/raw-transaction-class/neo/Core/TX/RawTransaction.py and let me know what you think? You can see from the commit description that I haven't included any coverage but I wanted to get your initial reaction before I kept working.
If you like what you see, I have not figured out how you would like to implement support for InvocationTransactions. My idea (as you can see here), is to implement a buildTokenTransfer function, but I am not sure what else should be created.
Most of the rest of the issue's wickets have been met, including validation (both in real time and via the Verify function).

[ixje]

At first glance looks good. I can't say if the function names might need to be renamed later on though to be inline with the rest of the exposed lightweight API. I had intentionally put #885 above this issue on the project board because I expected to learn from there the API's and naming style we want to support (I don't have the full picture as this point either).

Some random things I noticed in your current code:

• https://github.com/jseagrave21/neo-python/blob/raw-transaction-class/neo/Core/TX/RawTransaction.py#L164-L166 this will throw KeyError exceptions if the request fails and the balance key is not found. You can use something like if not bal.get('Balance', None) to avoid this. For the requests you'll either want to use .raise_for_status() or check the .status_code to be 200. If that fails we want to notify the user as it might be that their network is down. Right now we assume their balance is 0, which doesn't have to be the case.
• https://github.com/jseagrave21/neo-python/blob/raw-transaction-class/neo/Core/TX/RawTransaction.py#L140 have more specific exceptions. Now you have RawTXError everywhere and we need to parse the text to learn what's wrong. For the more specific exceptions that we throw we could provide a bit more information; e.g. " {len_new_attr} exceeds max attribute size {max_size}"
• I did not analyze in detail but for this: https://github.com/jseagrave21/neo-python/blob/raw-transaction-class/neo/Core/TX/RawTransaction.py#L336 I think there should be a way for the user to just sign by supplying their key. Without having to specify a path (you can use a temp path yourself)
• https://github.com/jseagrave21/neo-python/blob/raw-transaction-class/neo/Core/TX/RawTransaction.py#L359 throw an exception if it's not signed or they'll get useless data and will complain the TXID isn't right or doesn't exist on the chain. We can't expect users to read all docstrings, especially when the name is self explanatory.

[jseagrave21]

@ixje Thank you for the quick feedback. I will keep working on this and hopefully I will be able to merge it with the exposed lightweight API when it is ready.

[jseagrave21]

@ixje I made the corrections per your feedback and have now verified Contract and Claim Functionality. Please see the description for commit db1c1e7

[jseagrave21]

Token Transfers are now supported. See ac92993

[jseagrave21]

Importing raw transaction is now supported. See 4d78a5f

@ixje I think all of the original objectives for this issue have been met. Please review and let me know what you think. In the meantime, I can work on providing test coverage (I have manually tested every feature at this point).

[ixje]

Some things I believe we should do at minimum

• support use of custom neoscan servers (now it's all forced to api.neoscan.io). This should be configurable such that people can point it to their own instances (e.g. neolocal instance). Try to use a variable for the URL and create constants for the API endpoint locations. That way if we ever need to update an endpoint (e.g. /v1/get_balance/) we don't have to search and find all locations in the code.
• without looking at the code I would have a hard time differentiating between Exceeded max attribute size. and Max number of transaction attributes reached. (ref).
  I also think we can improve the error message by providing more information (see point 2 of Create (raw) Transaction builder class #886 (comment)). The first of the 2 could become something along the lines of Maximum description length exceeded ({cur_len} > {max_len}), the other e.g. Cannot add description attribute. Maximum transaction attributes ({MAX_ATTRIBUTES_CONST}) already reached.. The same holds for the url, remark etc attributes.
• I think buildClaim() is kind of odd naming. If I understand it correctly we'd do

tx = RawTransaction()
tx.TXType("claim")
tx.buildClaim("neo_addr")

To me, buildClaim suggests we're building a claim tx. Which we already know as specified by the TXType. I think an addClaim() makes more sense as you might have multiple claims in 1 TX (which I don't think is supported at this moment, more on this in point 1. below).

• there is no sign "command"

  neo-python/neo/Core/TX/RawTransaction.py

  Line 465 in 4d78a5f

  raise SignatureError(f"Transaction initiated, but the signature is incomplete. Use the `sign` command with the information below to complete signing.\n{json.dumps(context.ToJson(), separators=(',', ':'))}")

• there is no tx.Size(), should probably be self.Size()

  neo-python/neo/Core/TX/RawTransaction.py

  Line 517 in 4d78a5f

  raise TXFeeError(f'The tx size ({tx.Size()}) exceeds the max free tx size ({settings.MAX_FREE_TX_SIZE}).\nA network fee of {req_fee.ToString()} GAS is required.')

• can you explain the need for SerializeExclusiveData and DeserializeExclusiveData ? Any (de)serialization should be done via the respective (de)serialization calls of the TX Type. We do not want to maintain 2 places.

• Please streamline your function names. You mix camel case and Pascal case

Other things that we have to keep in mind until the other objective is realised:
(note: don't implement that now, the other objective should give a clear idea how to structure this)

1. We might want to not inherit from Transaction but just from object and turn the class into a real builder class. It now suggests to be a real NEO transaction type, which it is not. It would also allow re-use in the existing code where we can test on TX's being an instanceof ClaimTransaction etc. The TXType() could be dropped and made part of the constructor. It could then also support building multiple transactions in 1 single block E.g. 1 raw TX that sends multiple assets and claims gas and <insert action>
2. We have to assess if the current exception classes are sufficient. I see you've expanded them, but I can't judge if the granularity is low enough.

[lock9]

Hello,

I agree with @ixje, specially about this:

• support use of custom neoscan servers (now it's all forced to api.neoscan.io). This should be configurable such that people can point it to their own instances (e.g. neolocal instance). Try to use a variable for the URL and create constants for the API endpoint locations. That way if we ever need to update an endpoint (e.g. /v1/get_balance/) we don't have to search and find all locations in the code.

I think almost everyone should develop using their own private network, so I think this feature is absolutely important.

Also:

• I think the name "sourceAddress" is not the best, maybe some kind of "neoscanEndpoint", or equivalent (it's just compatible with neo-scan, I suppose);
• I would add the TxType into the constructor;
• Could you add signing with WIF too? Signing with NEP-2 consumes much more resources due to it's "brute-force protection".

I'm still having trouble when I use it in a loop statement, @jseagrave21 please contact me in slack! It's probably my fault, but I could use some help to find out what is going on.

[jseagrave21]

@ixje offline multi-sig is now fully supported and most feedback was implemented in ceb0918.
With @lock9's help, SourceAddress was split into Address and Network and the testnet is now the default network if Network is not implemented (see 54e4d73). At this point, I think most recommendations have been adjudicated. Please let me know if you have any more feedback.

[ixje]

I think that all we can address now is there 👍. I'm not so sure about Address as name, but I would have to use the script in a couple of scenario's first. Just leave it as is for now; we need to deal with #885 first before we can streamline naming. I'll copy the other outstanding "things to think about" below.

Other things that we have to keep in mind until the other objective is realised:
(note: don't implement that now, the other objective should give a clear idea how to structure this)

1. We might want to not inherit from Transaction but just from object and turn the class into a real builder class. It now suggests to be a real NEO transaction type, which it is not. It would also allow re-use in the existing code where we can test on TX's being an instanceof ClaimTransaction etc. The TXType() could be dropped and made part of the constructor. It could then also support building multiple transactions in 1 single block E.g. 1 raw TX that sends multiple assets and claims gas and
2. We have to assess if the current exception classes are sufficient. I see you've expanded them, but I can't judge if the granularity is low enough.