-
Notifications
You must be signed in to change notification settings - Fork 504
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
services/horizon: Add Muxed Account details to effect responses #3610
services/horizon: Add Muxed Account details to effect responses #3610
Conversation
b0be928
to
0399bd5
Compare
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.
Added some thoughts about whether or not we can unify code paths to always use the MuxedAccount
object.
ba9bec4
to
a623765
Compare
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.
LGTM! Added two questions/ideas.
// We abuse the details to inject muxed-account information without changing the DB schema | ||
details["account_muxed"] = address.Address() | ||
details["account_muxed_id"] = uint64(address.Med25519.Id) | ||
} |
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.
Sorry, this might have been discussed in previous PR but why not add a new column to the table? Adding a DEFAULT NULL
column is fast as far as I can remember.
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.
I discarded that because I agreed with @ire-and-curses to minimize schema changes.
That said, if we agree to make a schema change, it would also solve the performance problem with #3619
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.
ok, let's add the column. I don't like having to deserialise the whole envelope every time. Lesser of two evils.
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.
OK, I will merge this as is and then change the approach.
a623765
to
6bd8790
Compare
6bd8790
to
43c2435
Compare
Addresses part of #3591
Depends on #3603 (whose commits are included here for now)
TODO: