This repository has been archived by the owner on Nov 2, 2018. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 440
Conversation
This file contains 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
|
||
// SpendSiagSiafunds sends siafunds to another address. The siacoins stored in | ||
// the siafunds are sent to an address in the wallet. | ||
func (w *Wallet) SpendSiagSiafunds(amount types.Currency, dest types.UnlockHash, keyfiles []string) (txn types.Transaction, err error) { |
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.
these named return values are never used
you're right, this is super janky. I don't like any of it. But I don't really understand it either, so maybe it's not as bad as it looks. |
As far as 'it works without being buggy' goes, it should be pretty solid. It's just not a very strong design, nor is it much in the way of maintainability. |
ok, hopefully addressed all of your concerns |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
This PR feels like a hack job. The last hack job for a while, thankfully, but it builds on a lot of incomplete concepts. We need to get the siafunds spender out to our crowdfund holders asap, which is why I was okay building on top of deprecated designs.
One seemingly small change had major cascades. The genesis block outputs hadn't originally been included in the subscription list, which is a problem because that's where all the siafund outputs exist. So I added the genesis block to the notification list.
Problem is, this happens immediately, on startup. So, before the test suite ever mines a block, there's already a notification, and there needs to be a check that the notification spread to all of the other modules. This resulted in some headache, and ultimately a conclusion that the notification system could be revised.
Support for siag files has been added, but in a bit of a weird way. You don't load them into your wallet, because siag files are actually multisig files. The idea of multisig files is that you don't have everything in one place, yet you need to put everything in one place to spend the siafunds. Usage is complex and counterintuitive. Could use revising at some point, but I just accepted the limitations for now.
Siag stuff eventually needs to be fully integrated into the wallet itself.
There's no way to rescan the diff set when you add a new siafund key to the wallet. So, after adding the siafund key you just have to restart siad. That's pretty lame, but changing it would require some fairly major architectural changes. This just points back to the limitations of our current subscription architecture. I'll be looking at it more closely later this week as I return to polishing up the consensus package.
The api calls still aren't implemented for spending siafunds, because you might need to load multiple keys, instead of just one key. I figured that api call is something you can add when you implement the other calls in siac.
Ideally, the siafund stuff (including siac) will be merged into master sometime before tomorrow ends. I don't want to keep people waiting.