Open ixje opened 5 years ago
@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).
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:
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.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}"@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.
@ixje I made the corrections per your feedback and have now verified Contract and Claim Functionality. Please see the description for commit https://github.com/CityOfZion/neo-python/commit/db1c1e726b8b6a8e248aea9b8f20bb6f807e26ff
Token Transfers are now supported. See ac929933558aae5cdc0fc00f73d932bbca004c3f
Importing raw transaction is now supported. See 4d78a5f7ccac9493099919224c5c2062a34689d4
@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).
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 https://github.com/CityOfZion/neo-python/issues/886#issuecomment-464635952). 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" https://github.com/CityOfZion/neo-python/blob/4d78a5f7ccac9493099919224c5c2062a34689d4/neo/Core/TX/RawTransaction.py#L465
there is no tx.Size()
, should probably be self.Size()
https://github.com/CityOfZion/neo-python/blob/4d78a5f7ccac9493099919224c5c2062a34689d4/neo/Core/TX/RawTransaction.py#L517
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)
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>
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'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.
@ixje offline multi-sig is now fully supported and most feedback was implemented in ceb091821b0c07dec743c0e01a0eb73b68b8ac12.
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 54e4d730fa9db36026bb57a8d283cbc151bf1217). At this point, I think most recommendations have been adjudicated. Please let me know if you have any more feedback.
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 https://github.com/CityOfZion/neo-python/issues/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)
- 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
- 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.
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
Transaction
s 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 followsa simplified interface could be
Some ideas:
TransactionOutput
s (ref) using script hashes and alike. Instead we could support an API likecontract_tx.set_destination_addr("neo address")
CertUrl, DescriptionUrl, Description, Remark series
do not exceed 255 chars (see docs)ContractHash, ECDH series, Hash series
does not exceed 32 bytes (see docs)For the first iteration we should only support the following transactions