-
Notifications
You must be signed in to change notification settings - Fork 318
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
[chain] Consider Merging IndexedTxGraph and TxGraph into one thing #1147
Open
Labels
Comments
39 tasks
"the PR author" |
I moved this to alpha.4 since we want to get any functional changes in before a beta release. |
FWIW I think this is still a good idea. |
54 tasks
55 tasks
Can we postpone it to a 1.1 version @notmandatory? |
I think it is okay to postpone this to after 1.0.0. I think it may simplify the API and make it cleaner to use with less foot-guns. However, it is also not critical to have this. |
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
currently:
But it could also just be:
But we would have to give up the derive
PartialEq
onTxGraph
because I don't think we want to force indexes to implement it.The motivation is avoiding having to duplicate every mutating API on
TxGraph
toIndexedTxGraph
. We seem to have some trouble doing this. For example,IndexedTxGraph
andTxGraph
both haveinsert_tx
but they have different arguments atm but they should be the same. Also in #1041 I a method is being added toIndexedTxGraph
but it should also be onTxGraph
but the PR author forgot.The downside is that
TxGraph
would not longer be a pure data structure but since we don't use it like that I don't think this matters.The text was updated successfully, but these errors were encountered: