Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove Asset and AssetLink #982

Merged
merged 5 commits into from Dec 22, 2016

Conversation

Projects
None yet
4 participants
@sohkai
Copy link
Contributor

commented Dec 21, 2016

Fixes #974.

Moves some of the on-create validation to other model's init's and the one remaining static method to Transaction (where, arguably, it fits better).

@@ -1126,6 +980,40 @@ def __str__(self):
return Transaction._to_str(tx)

@staticmethod
def get_asset_id(transactions):

This comment has been minimized.

Copy link
@sohkai

sohkai Dec 21, 2016

Author Contributor

Moved from Asset.

@@ -40,20 +40,28 @@ def validate(self, bigchain):
inputs_defined = all([ffill.tx_input for ffill in self.fulfillments])

This comment has been minimized.

Copy link
@sohkai

sohkai Dec 21, 2016

Author Contributor

There's a bit of duplication in what I added to this method, but as @r-marques and I discussed, this method is somewhat confused (it's doing too much). Until we get a handle on when and what we should be validating in the server, I think this is OK like this.

@codecov-io

This comment has been minimized.

Copy link

commented Dec 21, 2016

Current coverage is 96.13% (diff: 97.36%)

Merging #982 into master will decrease coverage by 0.08%

@@             master       #982   diff @@
==========================================
  Files            35         35          
  Lines          1827       1787    -40   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits           1758       1718    -40   
  Misses           69         69          
  Partials          0          0          

Sunburst

Powered by Codecov. Last update 3a7fe30...9fc1b70

if condition.amount < 1:
raise AmountError('`amount` needs to be greater than zero')
output_amount += condition.amount
if any(any(condition.amount < 1 for condition in conditions) for

This comment has been minimized.

Copy link
@r-marques

r-marques Dec 21, 2016

Contributor

I don't think that this is needed.
You already validate the amounts of this transaction outputs in https://github.com/bigchaindb/bigchaindb/pull/982/files#diff-9f1ab5b5daf1b3072e2123b6939bfb07R51

And the input conditions where already validated when the input transation was submitted

This comment has been minimized.

Copy link
@sohkai

sohkai Dec 21, 2016

Author Contributor

You already validate the amounts

Ah, but that validation was only for CREATE and GENESIS transactions.

And the input conditions where already validated when the input transaction was submitted

Hmmm, actually that's a good point... the input conditions must be valid since we get them from valid blocks. I'll move the logic that you pointed out up and remove this bit.

@r-marques

This comment has been minimized.

Copy link
Contributor

commented Dec 21, 2016

In the end moving the types and amount validation code simplified the code 👍

@@ -598,20 +458,11 @@ def __init__(self, operation, asset, fulfillments=None, conditions=None,

self.version = version if version is not None else self.VERSION
self.operation = operation
self.asset = asset or Asset()
self.asset = asset

This comment has been minimized.

Copy link
@ssadler

ssadler Dec 22, 2016

Contributor

Yay

@sohkai sohkai merged commit 9319583 into master Dec 22, 2016

4 checks passed

codecov/patch 97.36% of diff hit (target 80.00%)
Details
codecov/project Absolute coverage decreased by -0.08% but relative coverage increased by +1.14% compared to 3a7fe30
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@vrde vrde deleted the fix/974/refactor-asset-asset-link branch Mar 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.