feat: Allow free request by GasPerToken is 0:0 #2798
Conversation
howjmay
force-pushed
the
gas-per-token
branch
9 times, most recently
from
0b1d30f
to
7263094
Compare
packages/util/ratio.go
Outdated
| func (ratio Ratio32) HasZeroComponent() bool { | ||
| return ratio.A == 0 || ratio.B == 0 | ||
| func (ratio Ratio32) HasInvalidZeroComponent() bool { | ||
| return (ratio.A == 0 || ratio.B == 0) && !(ratio.A == 0 && ratio.B == 0) | ||
| } |
There was a problem hiding this comment.
This funtion IMO now looks too specific for the gas ratio. The Ratio32 type is meant to represent any ratio, not just for gas fee (although right now the only use is for the gas fee).
I sugest moving this logic to the gas package:
package gas
var ZeroGasFee = Ratio32{}
func IsRatioValid(r Ratio32) bool {
return r == ZeroGasFee || !r.HasZeroComponent()
}
func IsGasFeeCharged(r Ratio32) bool {
return r != ZeroGasFee
}
There was a problem hiding this comment.
I have use IsRatioValid() to replace the current usage of HasZeroComponent() . Is it ok?
| @@ -140,6 +140,17 @@ func (reqctx *requestContext) checkAllowance() { | |||
| } | |||
|
|
|||
| func (reqctx *requestContext) shouldChargeGasFee() bool { | |||
| // freeGasPerToken checks whether we charge token per gas | |||
| // If it is free, then we will still burn the gas, but it doesn't charge tokens | |||
| // NOT FOR PUBLIC NETWORK | |||
There was a problem hiding this comment.
End users will not see this warning. Also there is no way for them to guess that they have to set 0:0 to disable the gas fee. IMO there should be a dedicated command in wasp-cli (like wasp-cli chain disable-gas-policy or whatever), or at least some example in the wiki.
There was a problem hiding this comment.
I have added the command and relating test
howjmay
force-pushed
the
gas-per-token
branch
2 times, most recently
from
4bad149
to
8045da8
Compare
howjmay
force-pushed
the
gas-per-token
branch
4 times, most recently
from
58f4db2
to
fbcc725
Compare
packages/util/ratio.go
Outdated
| var ZeroGasFee = Ratio32{} | ||
|
|
||
| func (ratio Ratio32) IsRatioValid() bool { | ||
| return ratio == ZeroGasFee || !ratio.HasZeroComponent() | ||
| } | ||
|
|
||
| func (ratio Ratio32) IsGasFeeCharged() bool { | ||
| return ratio != ZeroGasFee | ||
| } | ||
|
|
There was a problem hiding this comment.
Hey, that's better but still has a "separation of concerns" problem: Ratio32 is a generic math type. It can represent a ratio between any two integer values, for example bytes/network packets, cars/people, hamburgers/McDonald's stores...
So it should not know anything about gas, or anything ISC related. IMO these functions should go in the gas package.
packages/util/ratio.go
Outdated
| @@ -107,7 +107,7 @@ func (ratio *Ratio32) Read(r io.Reader) error { | |||
| rr := rwutil.NewReader(r) | |||
| ratio.A = rr.ReadUint32() | |||
| ratio.B = rr.ReadUint32() | |||
| if rr.Err == nil && ratio.HasZeroComponent() { | |||
| if rr.Err == nil && !(ratio == &Ratio32{} || !ratio.HasZeroComponent()) { | |||
There was a problem hiding this comment.
@dessaya here it has a cyclic importing problem. And we need to support 0:0 here too. Therefore, I have to do in this way. Is it ok?
There was a problem hiding this comment.
Well... I don't know. Again, this validation is specific to gas. From the point of view of Ratio32, 0:0 is invalid (since mathematically 0/0 is undefined). And both 0:x or x:0 are "dangerous" because they might panic withzero division error.
But OK, maybe the simplest solution is to add the concept of a "Zero" Ratio32, so the invariant of Ratio32 can be:
Either A != 0 and B != 0, or both A == 0 and B == 0.
Then:
func (ratio Ratio32) IsZero() bool {
return ratio.A == 0 && ratio.B == 0
}
func (ratio Ratio32) IsValid() bool {
return ratio.IsZero() || !ratio.HasZeroComponent()
}
Then:
| if rr.Err == nil && !(ratio == &Ratio32{} || !ratio.HasZeroComponent()) { | |
| if rr.Err == nil && ratio.IsValid() { |
Or just ignore all my comments and do what you thik is best. This is just me nitpicking... 😛
There was a problem hiding this comment.
Let me do the above way. That looks fine for me
howjmay
force-pushed
the
gas-per-token
branch
3 times, most recently
from
f054de0
to
73f5518
Compare
closes #2119