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

Implement SimplePolicyPlugin in Transaction.Verify #964

Merged
merged 32 commits into from
Jun 11, 2019

Conversation

jseagrave21
Copy link
Contributor

@jseagrave21 jseagrave21 commented Jun 10, 2019

What current issue(s) does this address, or what feature is it adding?

How did you solve this problem?
referenced logic used in #869 and logic I am using here https://github.com/jseagrave21/neo-python/blob/f75cb5dc58bedbfc12ea1a6afbca5332088f8ca6/neo/Core/TX/RawTransaction.py#L525-L541

How did you make sure your solution works?
make test and verification manually on my testnet node

Are there any special changes in the code that we should be aware of?
no

Please check the following, if applicable:

  • Did you add any tests?
  • Did you run make lint?
  • Did you run make test?
  • Are you making a PR to a feature branch or development rather than master?
  • Did you add an entry to CHANGELOG.rst? (if not, please do)

jseagrave21 and others added 30 commits October 9, 2018 20:38
Merge CoZ Development into jseagrave21 Development
Merge CoZ Development into jseagrave21 Development
Merge CoZ Development into jseagrave21 Development
* Update ExtendedJsonRpcApi.py

- add fix provided by @localhuman so original methods are returned as well as extended methods
…yOfZion#661)

* Add the option -u (unittest-net) to prompt.py

* Add unittest guildeline and add the smart contract source codes (UnitTest-SM.zip) to the fixtures package
for compatibility
* Fix ExtendedJsonRpcApi (CityOfZion#662)

* Update ExtendedJsonRpcApi.py

- add fix provided by @localhuman so original methods are returned as well as extended methods

* Mute expected test stacktrace and clearly identify why an exception is thrown. (CityOfZion#663)

* Add guideline for adding tests to the neo-privnet-unittest image (CityOfZion#661)

* Add the option -u (unittest-net) to prompt.py

* Add unittest guildeline and add the smart contract source codes (UnitTest-SM.zip) to the fixtures package

* Add raw transaction building examples (CityOfZion#665)

* Update neo-boa version to fix core building test
Merge CoZ Development into jseagrave21 Development
Merge CoZ Development into jseagrave21 Development
Merge CoZ Development into jseagrave21 Development
Merge CoZ Development into jseagrave21 development
Merge CoZ development into jseagrave21 development
- add tests for "showallaccounts" and "showallaccountstates"
Merge neo-python development into jseagrave21 development
Merge neo-python development into jseagrave21 development
Merge neo-python development into jseagrave21 development
Merge CoZ development into jseagrave21 development
Gracefully handle NEP-5 balance query failures (CityOfZion#744)
Merge CoZ development into neo-python development
Merge CoZ development into jseagrave21 development
Merge CoZ Development into jseagrave21 development
Merge CoZ/neo-python development into jseagrave21/neo-python development
@coveralls
Copy link

coveralls commented Jun 10, 2019

Coverage Status

Coverage increased (+0.04%) to 85.151% when pulling bde2df9 on jseagrave21:fix-tx-verify into 737477a on CityOfZion:development.

Copy link
Member

@ixje ixje left a comment

Choose a reason for hiding this comment

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

The solution does not seem to take the following rule into account (ref):

Deploy and invoke smart contract with neo-cli v2.10.2 will cost 0.001 GAS as the starting fee.

@jseagrave21
Copy link
Contributor Author

The solution does not seem to take the following rule into account (ref):

Deploy and invoke smart contract with neo-cli v2.10.2 will cost 0.001 GAS as the starting fee.

This was clarified in #dev-general on Discord by @vncoelho that this was only introduced in neo-cli and not a limitation on Seed Nodes or Concensus Nodes. It was discussed further and removed here: neo-project/neo-node#378

@ixje
Copy link
Member

ixje commented Jun 11, 2019

I missed that. Thanks for the link!
Looks good then 👍 👍

@vncoelho
Copy link

The implementation looks precise and the formula correct.

@ixje
Copy link
Member

ixje commented Jun 11, 2019

thanks for the extra check @vncoelho

@ixje ixje merged commit 7e29227 into CityOfZion:development Jun 11, 2019
Copy link

@vncoelho vncoelho left a comment

Choose a reason for hiding this comment

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

Maybe that if should be merged in the neocli as well.

fee = self.NetworkFee()
if self.Size() > settings.MAX_FREE_TX_SIZE and not self.Type == b'\x02': # Claim Transactions are High Priority
req_fee = Fixed8.FromDecimal(settings.FEE_PER_EXTRA_BYTE * (self.Size() - settings.MAX_FREE_TX_SIZE))
if req_fee < settings.LOW_PRIORITY_THRESHOLD:

Choose a reason for hiding this comment

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

This was the trick point. Nice job!

@jseagrave21 jseagrave21 deleted the fix-tx-verify branch June 11, 2019 14:57
@ixje
Copy link
Member

ixje commented Jun 12, 2019

100 points

@hal0x2328 hal0x2328 mentioned this pull request Jul 17, 2019
@lllwvlvwlll
Copy link
Member

lllwvlvwlll commented Jul 18, 2019

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

Successfully merging this pull request may close these issues.

6 participants