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.

Sign up for GitHub

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

Jump to bottom

SmartTransaction: Remove FirstSeenCompatibility #11472

Merged
merged 2 commits into from
Sep 14, 2023
Merged

SmartTransaction: Remove FirstSeenCompatibility #11472

merged 2 commits into from
Sep 14, 2023

Conversation

kiminuo
Copy link
Collaborator

@kiminuo kiminuo commented Sep 12, 2023 •
edited

This PR would simplify #11421.

I'm not totally sure if this is correct but:

  • AFAIK we use SmartTransaction and its SmartTransaction.FromLine() and SmartTransaction.ToLine() to serialize and deserialize SmartTransactions.
  • I can find JSON serialization for SmartTransactions only in tests, not in production code.
  • Still a 3rd party can depend on actual JSON serialization of SmartTransaction but personally I doubt it.

WDYT?

lontivero
lontivero reviewed Sep 12, 2023
View reviewed changes
Copy link
Collaborator

@lontivero lontivero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems correct to me.

turbolay
turbolay approved these changes Sep 12, 2023
View reviewed changes
Copy link
Collaborator

@turbolay turbolay left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see no problems with this PR and nobody seems to use this serialization according to answers

@kiminuo
Copy link
Collaborator Author

kiminuo commented Sep 12, 2023

Ondrej and Mr. Kukks said that they are OK with this change.

@kiminuo kiminuo marked this pull request as ready for review September 12, 2023 15:46
adamPetho
adamPetho approved these changes Sep 12, 2023
View reviewed changes
Copy link
Collaborator

@adamPetho adamPetho left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kiminuo kiminuo mentioned this pull request Sep 12, 2023
Merged
@kiminuo kiminuo merged commit b9e8593 into WalletWasabi:master Sep 14, 2023
7 checks passed
@kiminuo kiminuo deleted the feature/2023-09-12-SmartTransaction-remove-compatibility-code branch September 14, 2023 06:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@turbolay turbolay turbolay approved these changes

@adamPetho adamPetho adamPetho approved these changes

@lontivero lontivero Awaiting requested review from lontivero

Assignees
No one assigned
Labels
size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@kiminuo @lontivero @turbolay @adamPetho