-
Notifications
You must be signed in to change notification settings - Fork 99
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
GasTracker split #202
GasTracker split #202
Conversation
…out reward features
|
||
// Params implements the types.QueryServer interface. | ||
func (s *QueryServer) Params(c context.Context, request *types.QueryParamsRequest) (*types.QueryParamsResponse, error) { | ||
if request == nil { |
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 think we can avoid checking for this as the gRPC stub should handle this seamlessly
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.
This is done in the Cosmos SDK so I did the same 🤷♂️
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.
Reviewed tracking part, LGTM.
… gas limit (unlike calculating it from Txs)
e2e: chain options added
…imited (LT 1.0) as 1.0 triggers an error on x/mint BeginBlocker
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.
rewards protos ACK.
if err := dfd.bankKeeper.SendCoinsFromAccountToModule(ctx, acc.GetAddress(), authTypes.FeeCollectorName, fees); err != nil { | ||
return sdkErrors.Wrapf(sdkErrors.ErrInsufficientFunds, err.Error()) | ||
} | ||
return nil |
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.
We need to be conscious about this approach for tracking pruning, because tracking can have records of this TX whilst rewards does not.
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.
Also side note, we're fetching the Dec
twice here, for performance we could just fetch the tx fee rebate ratio here, and apply the isZero()
logic here.
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.
Dec
usage fixed. As for pruning. If there were no TxRebate rewards a corresponding rewards tracking entry is not created and this is handled by the x/rewards
distribution logic.
…; Ante handlers reordered; ContractMetadata has ContractAddress in it. Tests fixes
x/rewards, x/tracking: state tests added. E2E tests: Sudo test enabled (after the wasmd upgrade), Stargate query test disabled (removed by the wasmvm developers).
} | ||
|
||
// Check that only one coin has been minted | ||
if len(dappRewards) != 1 { |
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.
Why are we making an assumption here that only one coin can be minted?
I understand current version of cosmos sdk only support one coin for inflation, but that could change in future. Besides, if we go by that then input will always contain one coin anyway. So there is no point in this check.
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.
@fdymylja Thoughts?
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.
Copied from your code so need yours feedback.
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.
This mocks the behaviour of the current minting module in the SDK, once we upgrade to a different SDK version if there are changes in the minting module we can change this.
… the stacks is empty; ModuleToModule checks for empty coins added
// estimateBlockRewards update block distribution state with tracked rewards calculating reward shares per contract. | ||
// Func iterates over all tracked transactions and estimates inflation (on block level) and fee rebate (merging | ||
// tokens for each transaction contract has operation at) rewards for each contract. | ||
func (k Keeper) estimateBlockRewards(ctx sdk.Context, blockDistrState *blockRewardsDistributionState) *blockRewardsDistributionState { |
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'm a bit concerened on GC overhead of passing a pointer, golang pointers are a bit quirky since it passes pointers to heap which takes longer to clean than stack values, the GC cycle is probably longer than block times so it would accumulate over time.
Once that is done the GC has to dereference which is a costly implementation in itself.
This might be over panicking however, in case performance analysis is neccesary in the future we can always make a escape analysis build with go build -gcflags="-m"
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.
You are right about pointers in general. Goway is to be as immutable as possible. But here, since we are not "escaping" (not in terms of the compiler) the DistributeRewards
func, this is kind of OK. But if that really bothers you, I can pass an object and return an updated one. BTW maps in the State object are "heaped" (probably 🤷♂️).
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.
Let's not get ahead of ourselves here, If we switch to rewards distribution using a smart contract then this change would be moot. I'll just keep a mental note on it as we observe further growth.
P.D: yeah maps are notoriously bad in golang for memory growth more on this golang issue
Co-authored-by: Eduardo Díaz <edjroz@users.noreply.github.com>
Codecov Report
@@ Coverage Diff @@
## main #202 +/- ##
==========================================
+ Coverage 56.68% 63.45% +6.77%
==========================================
Files 20 47 +27
Lines 1302 1929 +627
==========================================
+ Hits 738 1224 +486
- Misses 527 664 +137
- Partials 37 41 +4
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
No description provided.