select coins for delegation simplifications #2254
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The code was becoming quite convoluted here with function names that were screaming for refactoring (when we start adding `'` to functions, it's usually a good sign that something can be simplified). The main change is actually in 'selectCoinsExternal' which is now parameterized over the selection to run. This way, the steps of assigning change addresses are factored out in this function and a lot of the duplication goes away. Another important change is that I've moved the signing and submission of join/quit outside of the body of the function. So that each step can be ran independently. This avoid the need for weird intermediate product types aggregating more and more information. Now, both functions are returning a 'DelegationAction' and merely checking that a join or quit is possible.
KtorZ
added
the
Feature
Contributor
|
I don't really understand the changes from skimming over it. I'd have to have a deeper look at it if you require a detailed review, otherwise I'm fine with merging. |
Member
Author
|
@hasufell I have tried to explain the main changes in my PR comment. Did you read it? Do you have any questions or something I could explain? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Issue Number
#2200
Overview
16a5f0c
📍 remove duplicated logic with regards to selections for join/quit
The code was becoming quite convoluted here with function names that
were screaming for refactoring (when we start adding
'to functions,it's usually a good sign that something can be simplified).
The main change is actually in 'selectCoinsExternal' which is now
parameterized over the selection to run. This way, the steps of
assigning change addresses are factored out in this function and a lot
of the duplication goes away.
Another important change is that I've moved the signing and submission
of join/quit outside of the body of the function. So that each step
can be ran independently. This avoid the need for weird intermediate product
types aggregating more and more information. Now, both functions are
returning a 'DelegationAction' and merely checking that a join or quit
is possible.
4a5f4d9
📍 return structured error for invalid wallets, instead of plain string
Comments