Skip to content
This repository has been archived by the owner on Nov 15, 2021. It is now read-only.

Create (raw) Transaction builder class #886

Open
ixje opened this issue Feb 13, 2019 · 10 comments
Open

Create (raw) Transaction builder class #886

ixje opened this issue Feb 13, 2019 · 10 comments

Comments

@ixje
Copy link
Member

ixje commented Feb 13, 2019

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

@jseagrave21
Copy link
Contributor

@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
Copy link
Member Author

ixje commented Feb 18, 2019

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:

@jseagrave21
Copy link
Contributor

@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 pushed a commit to jseagrave21/neo-python that referenced this issue Feb 20, 2019
@jseagrave21
Copy link
Contributor

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

@jseagrave21
Copy link
Contributor

jseagrave21 commented Feb 24, 2019

Token Transfers are now supported. See ac92993

@jseagrave21
Copy link
Contributor

jseagrave21 commented Feb 24, 2019

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
Copy link
Member Author

ixje commented Feb 25, 2019

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"

    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()

    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
Copy link

lock9 commented Feb 25, 2019

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 pushed a commit to jseagrave21/neo-python that referenced this issue Feb 26, 2019
- every comment from CityOfZion#886 (comment) is addressed
  - since each instance is a RawTransaction, I needed to reference the unique methods from the ClaimTransaction and InvocationTransaction classes, which includes SerializeExclusiveData and DeserializeExclusiveData

- from CityOfZion#886 (comment)
  - I have not yet found a suitable replacement for the name `SourceAddress`
  - I have not yet found a suitable method for integrating the TxType into the constructor as I am unsure where I would put TransactionType.ContractTransaction
  - WIF is already supported. Swapped variable name to NEP2orWIF per feedback
jseagrave21 pushed a commit to jseagrave21/neo-python that referenced this issue Mar 1, 2019
- every comment from CityOfZion#886 (comment) is addressed
  - since each instance is a RawTransaction, I needed to reference the unique methods from the ClaimTransaction and InvocationTransaction classes, which includes SerializeExclusiveData and DeserializeExclusiveData

- from CityOfZion#886 (comment)
  - I have not yet found a suitable replacement for the name `SourceAddress`
  - I have not yet found a suitable method for integrating the TxType into the constructor as I am unsure where I would put TransactionType.ContractTransaction
  - WIF is already supported. Swapped variable name to NEP2orWIF per feedback
jseagrave21 pushed a commit to jseagrave21/neo-python that referenced this issue Mar 1, 2019
- every comment from CityOfZion#886 (comment) is addressed
  - since each instance is a RawTransaction, I needed to reference the unique methods from the ClaimTransaction and InvocationTransaction classes, which includes SerializeExclusiveData and DeserializeExclusiveData

- from CityOfZion#886 (comment)
  - I have not yet found a suitable replacement for the name `SourceAddress`
  - I have not yet found a suitable method for integrating the TxType into the constructor as I am unsure where I would put TransactionType.ContractTransaction
  - WIF is already supported. Swapped variable name to NEP2orWIF per feedback
jseagrave21 pushed a commit to jseagrave21/neo-python that referenced this issue Mar 1, 2019
- every comment from CityOfZion#886 (comment) is addressed
  - since each instance is a RawTransaction, I needed to reference the unique methods from the ClaimTransaction and InvocationTransaction classes, which includes SerializeExclusiveData and DeserializeExclusiveData

- from CityOfZion#886 (comment)
  - I have not yet found a suitable replacement for the name `SourceAddress`
  - I have not yet found a suitable method for integrating the TxType into the constructor as I am unsure where I would put TransactionType.ContractTransaction
  - WIF is already supported. Swapped variable name to NEP2orWIF per feedback
jseagrave21 pushed a commit to jseagrave21/neo-python that referenced this issue Mar 1, 2019
- every comment from CityOfZion#886 (comment) is addressed
  - since each instance is a RawTransaction, I needed to reference the unique methods from the ClaimTransaction and InvocationTransaction classes, which includes SerializeExclusiveData and DeserializeExclusiveData

- from CityOfZion#886 (comment)
  - I have not yet found a suitable replacement for the name `SourceAddress`
  - I have not yet found a suitable method for integrating the TxType into the constructor as I am unsure where I would put TransactionType.ContractTransaction
  - WIF is already supported. Swapped variable name to NEP2orWIF per feedback
jseagrave21 pushed a commit to jseagrave21/neo-python that referenced this issue Mar 1, 2019
- every comment from CityOfZion#886 (comment) is addressed
  - since each instance is a RawTransaction, I needed to reference the unique methods from the ClaimTransaction and InvocationTransaction classes, which includes SerializeExclusiveData and DeserializeExclusiveData

- from CityOfZion#886 (comment)
  - I have not yet found a suitable replacement for the name `SourceAddress`
  - I have not yet found a suitable method for integrating the TxType into the constructor as I am unsure where I would put TransactionType.ContractTransaction
  - WIF is already supported. Swapped variable name to NEP2orWIF per feedback
@jseagrave21
Copy link
Contributor

@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
Copy link
Member Author

ixje commented Mar 4, 2019

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants