-
Notifications
You must be signed in to change notification settings - Fork 24
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
Problem: Need to start refactoring the transaction-related code (BEP-9) #33
base: master
Are you sure you want to change the base?
Conversation
9/README.md
Outdated
|
||
1. Move the code for checking the MongoDB-specific things (listed in https://github.com/bigchaindb/BEPs/blob/master/13/README.md#bigchaindb-server-deviations ) from the webserver code to the transaction-validation code. Maybe only do those checks if the backend database is MongoDB? | ||
1. Use a phrase other than "pluggable consensus" for that feature. See [issue #1779](https://github.com/bigchaindb/bigchaindb/issues/1779). | ||
1. Seperate the code for checking transaction validity from the code for converting a transaction object to/from a Python dict. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean that we keep all the objects (transaction, outpts etc) in dict form and carry out the validation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It just means that the code for converting a dict to an object of class Transaction shouldn't also do transaction validation as a hidden side effect. The transaction validation code should be something separate. Some transaction validation could be done before the conversion to an object (e.g. JSON Schema validation) and some could be done after. It's possible that the conversion might fail even if the JSON string or dict passes JSON Schema validation. I'm not sure. It might not be possible, and if it is, then I suspect it's a weird edge case.
I will add this explanation to the BEP itself, so that it's clearer.
Would it also make sense to de-couple database dependent validation from database independent validation in this phase? |
Yes, that might be a nice, single-pull-request bit of refactoring. I will add it as another task in the list. |
@kansi Did my changes resolve your comments? @vrde Implementing this BEP can happen after the release of BigchainDB 2.0 stable, so you can wait until then to review it, but I do want your feedback on it before we merge it, since you'll almost certainly be involved in the transaction code refactoring effort. |
I'm wondering how much effort we should put in refactoring the validation code, especially now that Crypto Conditions aren't part of the Interledger Protocol (ILP) any more. Crypto Conditions didn't help acquisition (writing a driver for BigchainDB requires a working implementation of that standard) and both our Python implementation and the integration with BigchainDB (I'm 👀 at you Basically, I would not spend much time in refactoring the validation code. It would require rewriting parts of CryptoConditions and BigchainDB. Instead, we can do some work to encapsulate our logic for validating version 2 transactions and put more effort in rethinking how a new, simpler transaction model would look like. The encapsulation process looks like this: The steps are:
|
@vrde Okay, I can revise BEP-9 to incorporate those ideas. The main idea seems to be to move (not delete) all the existing transaction validation code (for v2 transactions) into a module, so that it can be treated as a validation plugin (or a set of plugins). Other plugins could validate v3 transactions or add other network-specific business logic (such as RBAC). That alone might be quite tricky, because today transaction validation and transaction construction are all mixed up. We can start thinking about what v3 transactions will look like (e.g. no more crypto conditions), but that's a separate issue. This BEP is mostly about setting the stage so that adding support for v3 transactions (or other new validation logic) is fairly easy. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trying to remove this PR from the "Review requests" panel.
Solution: Propose BEP-9: Transaction Code Refactoring, Phase 1
I'm not sure if this should be a "Standard" or an "Informational" BEP. I put "Standard" for now.