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

Problem: Need to start refactoring the transaction-related code (BEP-9) #33

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

ttmc
Copy link
Contributor

@ttmc ttmc commented Apr 20, 2018

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.

@ttmc ttmc self-assigned this Apr 20, 2018
@ttmc ttmc requested review from vrde and kansi April 20, 2018 06:38
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.
Copy link
Contributor

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?

Copy link
Contributor Author

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.

@kansi
Copy link
Contributor

kansi commented Apr 20, 2018

Would it also make sense to de-couple database dependent validation from database independent validation in this phase?

@ttmc
Copy link
Contributor Author

ttmc commented Apr 21, 2018

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.

@ttmc
Copy link
Contributor Author

ttmc commented Apr 24, 2018

@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.

@ttmc ttmc mentioned this pull request May 7, 2018
@ttmc ttmc changed the title BEP-9: Transaction Code Refactoring, Phase 1 Problem: Need to start refactoring the transaction-related code (BEP-9) May 14, 2018
@vrde
Copy link
Contributor

vrde commented Jul 24, 2018

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 bigchaindb.models and bigchaindb.common) have performance issues.

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:
giphy

The steps are:

  • Define a validation interface that can be reused by the "validation plugins".
  • Isolate the current validation code in a specific module.
  • Have a façade to interface with the legacy validation code.

@ttmc
Copy link
Contributor Author

ttmc commented Jul 24, 2018

@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.

@ttmc ttmc added the new BEP label Aug 7, 2018
Copy link
Contributor

@vrde vrde left a 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.

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

Successfully merging this pull request may close these issues.

3 participants