Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
txnbuild: Reenable Multiplexed strkeys (SEP23 M-addresses) behind flags #3527
txnbuild: Reenable Multiplexed strkeys (SEP23 M-addresses) behind flags #3527
Changes from 21 commits
efb9c3b
a0bf286
27a79c6
14ec0cb
8546938
f862efb
0742e88
3533829
d8e1e2a
29bf10c
828be10
f77f352
3b5df73
32073d9
5010f35
10c5ed4
265b601
85a33de
afa16c3
0a6beda
4bad428
8caa781
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Nit: I know this PR has been reviewed multiple times but I have one (simple) idea how to improve the API. Passing bool value to methods can be misleading (user need to check the description of the method to understand what is the param). We can create additional extra methods for each method accepting
withMuxedAccounts
.BuildXDR
as an example:BuildXDR(withMuxedAccounts bool) (xdr.Operation, error)
tobuildXDR(withMuxedAccounts bool) (xdr.Operation, error)
BuildXDR()
stays in a form before this PR. CallsbuildXDR(false)
internally.BuildXDRWithMuxedAccounts()
is added and callsbuildXDR(true)
internally.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.
@bartekn that's exactly what I implemented originally and I discarded after the design discussion. Please see 27a79c6#diff-0ef2e7d165d2f196939c937afadef91206823b0671f32120b265843fd291332aR15 and the issue body edit history (which originally presented the same design you suggested, symbol-naming aside).
We should be more cohesive when discussing designs (but that's for a retro discussion). The purpose of creating an early draft a week ago was exactly avoiding this type of situation.
I personally think this needs to go out (it's already late) but if @ire-and-curses thinks it's worth another rewrite, I will cave in and restore my original design.
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.
@ire-and-curses In order to get more context on what happened:
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.
Let's discuss in team meeting today. I really want to get this out.
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.
We decided offline to keep it as it is. Thanks for the suggestion though @bartekn