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

Refactor TransactionFrame to be immutable #2163

Open
jonjove opened this issue Jun 20, 2019 · 2 comments · May be fixed by #4307
Open

Refactor TransactionFrame to be immutable #2163

jonjove opened this issue Jun 20, 2019 · 2 comments · May be fixed by #4307
Assignees
Labels
cleanup refactoring or other internal improvements

Comments

@jonjove
Copy link
Contributor

jonjove commented Jun 20, 2019

TransactionFrame and TxSetFrame have interfaces that do not satisfy the criteria of "easy to use correctly and hard to use incorrectly". Specifically, these classes would be much more intuitive if they were immutable.

@MonsieurNicolas
Copy link
Contributor

I think for both, we should discern production and testing use cases:
for prod, everything should be immutable, so "results" (in transactionframe) for example, should not be a member variable.
for testing, it's fine to have mutability, but we should make sure to isolate those member variable/functions with the #ifdef BUILD_TESTS (or better, move the functionality in test only classes, like it may actually be convenient to have a "transactionframe" equivalent that has both a transaction and its result).

@MonsieurNicolas MonsieurNicolas added this to To do in v19.0.0 via automation Mar 31, 2022
@MonsieurNicolas MonsieurNicolas moved this from To do to In progress in v19.0.0 Mar 31, 2022
@jayz22 jayz22 mentioned this issue Apr 11, 2022
6 tasks
@MonsieurNicolas MonsieurNicolas removed this from In progress in v19.0.0 Apr 21, 2022
@MonsieurNicolas MonsieurNicolas added this to To do in v19.1.0 via automation Apr 21, 2022
@MonsieurNicolas MonsieurNicolas moved this from To do to In progress in v19.1.0 Apr 21, 2022
@MonsieurNicolas MonsieurNicolas removed this from In progress in v19.1.0 Jun 1, 2022
@MonsieurNicolas MonsieurNicolas added the cleanup refactoring or other internal improvements label Oct 4, 2022
@MonsieurNicolas MonsieurNicolas changed the title Refactor TransactionFrame and TxSetFrame Refactor TransactionFrame to be immutable Oct 4, 2022
@MonsieurNicolas
Copy link
Contributor

Work on TxSetFrame was done.

Work on TransactionFrame??? is left.
There might be more to it than just TransactionResult -> easiest might be to move everything mutable into a TxResultContext object (that contains TransactionResult and also things like mCachedAccount).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup refactoring or other internal improvements
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants