Add RequiredFee field to Check/DeliverResult #357
Conversation
|
@ethanfrey in my opinion best way to resolve it is to implement getter methods for whatever is needed and pass an interface to deliver instead of x.Coin directly. Then the deliver result won't care about the actual type and we can still get the juice. And yeah, in concrete decorator we can cast the type. |
|
The point is we need to get and set this in many places.... And I can't even define type interface Getter {
GetFee() x.Coin
}due to the imports... I really want to set a coin on it, but it currently cannot know what a coin is... pulling out coins in a PR now |
|
I mean, why can't this be an interface{} or a type that implements some methods, like GetWhole() GetFractional() GetTicker() and then you can cast it in some other places you need to. could be a Currency interface or something |
Extract coin package
Codecov Report
@@ Coverage Diff @@
## master #357 +/- ##
========================================
+ Coverage 70.3% 70.4% +<.1%
========================================
Files 127 127
Lines 6536 6554 +18
========================================
+ Hits 4600 4616 +16
- Misses 1462 1463 +1
- Partials 474 475 +1
Continue to review full report at Codecov.
|
|
Now that the package for holding coins is called I cannot propose anything better although my thinking goes in the direction of something like func x(amount coin.Coin) {} // now, a single ticker value
func x(amount coin.Single) {} // after renaming
func x(amount coin.Coins) {} // now, multiple ticker values
func x(amount coin.Multiple) {} // after renamingNow that the package is called total, err := coins.Sum(amount, tax, fee) |
abci.go
Outdated
| // Log is human-readable informational string | ||
| Log string | ||
| // RequiredFee can set an custom fee that must be paid for this transaction to be allowed to run. | ||
| // This is enforced in cash.DynamicFeeDecorator |
There was a problem hiding this comment.
I think changing the phrasing from this is enforced to this might be enforced by for example might explain better. Although today this field is added specifically for the use by the cash.DynamicFeeDecorator, anyone should be able to use it. Otherwise it sounds like this is a hard dependency. 💅
I love the comments though. They explain a lot! 🏅
abci.go
Outdated
| // Log is human-readable informational string | ||
| Log string | ||
| // RequiredFee can set an custom fee that must be paid for this transaction to be allowed to run. | ||
| // This is enforced in cash.DynamicFeeDecorator |
There was a problem hiding this comment.
This is similar to the discussion on https://github.com/iov-one/weave/pull/357/files#diff-dc931a8b7120348333d7b68dce1ca6ffR44
|
I will update docs. And yes, let's make a new issue for coin package cleanup. I just pulled it out into a package to compile properly. Would like more input before renaming everything. |
|
About renaming, this breaks lots of protobuf definitions. I would almost prefer to keep Coin and Coins, but the other functions can easily be renamed. And also, the package could have another name easily |
Addresses #349
This just adds the fields, must be rebased once #348 is closed and then enforce the required fee in the DynamicFeeHandler.
First pass, to add
x.Cointo theweave.DeliverResultcreates an import cycle. weave -> x -> weaveThis repo was designed that modules only import from their parent modules to avoid cycles, and that we use interfaces defined in the top level rather than reference implementations from sibling modules. There are two main exceptions,
errorsandcrypto, which are generic libraries and can be imported by any package, in addition toweaveitself.Anyway, if we now add
Cointo a project-wide interface, I see three options:xtoweaveand update all importscoinswithcoin.go,coins.goetc and treat that as a library package likecryptoanderrorsConclusion I put everything in the
coinpackage and changed tons of imports...