Add local cache inside the takerfee #8148
Conversation
ValarDragon
added
V:state/compatible/backport
ValarDragon
requested review from
czarcas7ic,
p0mvn,
mattverse and
AlpinYukseloglu
as code owners
|
Warning Rate Limit Exceeded@ValarDragon has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 28 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe changes in the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
x/poolmanager/taker_fee.go
Outdated
| defaultTakerFeeBz := k.paramSpace.GetRaw(ctx, types.KeyDefaultTakerFee) | ||
| if !bytes.Equal(defaultTakerFeeBz, k.defaultTakerFeeBz) { | ||
| var defaultTakerFeeValue sdk.Dec | ||
| json.Unmarshal(defaultTakerFeeBz, &defaultTakerFeeValue) | ||
| k.defaultTakerFeeVal = defaultTakerFeeValue | ||
| } | ||
| return k.defaultTakerFeeVal |
There was a problem hiding this comment.
Ensure thread safety in caching mechanism.
The caching mechanism in GetDefaultTakerFee does not appear to be thread-safe. Consider using synchronization techniques like mutexes to protect the shared state (defaultTakerFeeBz and defaultTakerFeeVal) from concurrent access issues.
| defaultTakerFeeBz []byte | ||
| defaultTakerFeeVal sdk.Dec |
There was a problem hiding this comment.
Initialization of new fields in Keeper struct.
The new fields defaultTakerFeeBz and defaultTakerFeeVal in the Keeper struct are not initialized in the NewKeeper constructor. This might lead to nil pointer dereferences or unexpected behavior. Ensure these fields are properly initialized when a new Keeper instance is created.
There was a problem hiding this comment.
Actionable comments posted: 0
Out of diff range and nitpick comments (9)
CHANGELOG.md (9)
Line range hint
1002-1002: Heading levels should only increment by one level at a time.
Line range hint
1014-1014: Heading levels should only increment by one level at a time.
Line range hint
1023-1023: Heading levels should only increment by one level at a time.
Line range hint
1028-1028: Heading levels should only increment by one level at a time.
Line range hint
1080-1080: Heading levels should only increment by one level at a time.
Line range hint
341-341: Trailing punctuation in heading.
Line range hint
604-604: Bare URL used.
Line range hint
662-662: Bare URL used.
Line range hint
1243-1243: No empty links.
| @@ -34,6 +34,9 @@ type Keeper struct { | |||
| poolModules []types.PoolModuleI | |||
|
|
|||
| paramSpace paramtypes.Subspace | |||
|
|
|||
| defaultTakerFeeBz []byte | |||
There was a problem hiding this comment.
We need to initialize this value somewhere right?
There was a problem hiding this comment.
no its intended to be set to nil until initialization!
| k.paramSpace.Get(ctx, types.KeyDefaultTakerFee, &defaultTakerFee) | ||
| return defaultTakerFee | ||
| defaultTakerFeeBz := k.paramSpace.GetRaw(ctx, types.KeyDefaultTakerFee) | ||
| if !bytes.Equal(defaultTakerFeeBz, k.defaultTakerFeeBz) { |
There was a problem hiding this comment.
ditto here for the value for k.defaultTakerFeeBz
There was a problem hiding this comment.
I think this is still wrong though, which was highlighted as I was stepping through with debugger. Don't we want to set k.defaultTakerFeeBz to defaultTakerFeeBz at the end? Otherwise this will always hit the if statement. Its not "wrong", but we are forced to unmarshal the value every time which defeats the purpose.
There was a problem hiding this comment.
Actionable comments posted: 0
Out of diff range and nitpick comments (9)
CHANGELOG.md (9)
Line range hint
1016-1016: Correct the heading level for better document structure.- #### [v6.4.0] + ### [v6.4.0]
Line range hint
1028-1028: Correct the heading level for better document structure.- #### [v6.3.1] + ### [v6.3.1]
Line range hint
1037-1037: Correct the heading level for better document structure.- #### [v6.3.0] + ### [v6.3.0]
Line range hint
1042-1042: Correct the heading level for better document structure.- #### [v6.2.0] + ### [v6.2.0]
Line range hint
1094-1094: Correct the heading level for better document structure.- #### [v6.1.0] + ### [v6.1.0]
Line range hint
355-355: Remove the trailing punctuation in the heading for consistency.- ## [v4.0.0] + ## [v4.0.0]
Line range hint
618-618: Convert the bare URL to a markdown link for better readability.- See: [SDK v0.43.0 Release Notes](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.43.0) For more details + See: [SDK v0.43.0 Release Notes](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.43.0) for more details
Line range hint
676-676: Convert the bare URL to a markdown link for better readability.- Upgrade instructions for node operators can be found [here](https://github.com/osmosis-labs/osmosis/blob/v5.x/networks/osmosis-1/upgrades/v5/guide.md) + Upgrade instructions for node operators can be found [here](https://github.com/osmosis-labs/osmosis/blob/v5.x/networks/osmosis-1/upgrades/v5/guide.md).
Line range hint
1257-1257: Ensure all links are functional and not empty.Please verify that the link provided is correct and accessible.
There was a problem hiding this comment.
Actionable comments posted: 1
Out of diff range and nitpick comments (1)
x/poolmanager/taker_fee.go (1)
Line range hint
107-107: Prioritize optimization ofchargeTakerFee.The TODO comment in
chargeTakerFeementions the need to optimize this function for gas and CPU usage. Given the critical nature of fee handling in transaction systems, prioritizing this optimization could lead to significant performance improvements.
|
This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you! |
There was a problem hiding this comment.
Actionable comments posted: 0
Out of diff range and nitpick comments (9)
CHANGELOG.md (9)
Line range hint
1025-1025: Correct the heading level to maintain a proper hierarchical structure.- ### [v6.4.0] + ## [v6.4.0]
Line range hint
1037-1037: Correct the heading level to maintain a proper hierarchical structure.- ### SDK fork updates + ## SDK fork updates
Line range hint
1046-1046: Correct the heading level to maintain a proper hierarchical structure.- ### Minor improvements & Bug Fixes + ## Minor improvements & Bug Fixes
Line range hint
1051-1051: Correct the heading level to maintain a proper hierarchical structure.- ### Features + ## Features
Line range hint
1103-1103: Correct the heading level to maintain a proper hierarchical structure.- ### SDK fork updates + ## SDK fork updates
Line range hint
364-364: Remove the trailing punctuation in the heading for consistency.- ## [v6.0.0]. + ## [v6.0.0]
Line range hint
627-627: Convert the bare URL to a markdown link for better readability.- See: [SDK v0.43.0 Release Notes](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.43.0) For more details + See: [SDK v0.43.0 Release Notes](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.43.0) for more details
Line range hint
685-685: Convert the bare URL to a markdown link for better readability.- [wasmd-v.022.0-osmo-v7.2](https://github.com/osmosis-labs/wasmd/releases/tag/v0.22.0-osmo-v7.2) Upgrade SDK and IAVL dependencies to use fast storage + [wasmd-v.022.0-osmo-v7.2](https://github.com/osmosis-labs/wasmd/releases/tag/v0.22.0-osmo-v7.2) upgrade SDK and IAVL dependencies to use fast storage
Line range hint
1266-1266: Ensure there are no empty links in the document.- [v1.0.2](https://github.com/osmosis-labs/osmosis/releases/tag/v1.0.2) - 2021-06-18 + [v1.0.2](https://github.com/osmosis-labs/osmosis/releases/tag/v1.0.2) - 2021-06-18 This release improves the CLI UX of creating and querying gauges.
* Add local cache inside the takerfee * add error handling * Add Changelog * Fix cache per @adam's comment * Update x/poolmanager/taker_fee.go (cherry picked from commit b8fd642) Co-authored-by: Dev Ojha <[email protected]>
* Add local cache inside the takerfee (#8148) * Add local cache inside the takerfee * add error handling * Add Changelog * Fix cache per @adam's comment * Update x/poolmanager/taker_fee.go (cherry picked from commit b8fd642) # Conflicts: # CHANGELOG.md * chore: fix merge errors --------- Co-authored-by: Dev Ojha <[email protected]> Co-authored-by: PaddyMc <[email protected]>
What is the purpose of the change
Remove parameter unmarshal time from charging TakerFee from poolmanager
Testing and Verifying
Covered by existing unit tests
Documentation and Release Note
Unreleasedsection ofCHANGELOG.md? - DONEWhere is the change documented?
x/{module}/README.md)Summary by CodeRabbit