chore(marketplace): add a cache for storage requests #1090
Conversation
2-towns
changed the title
codex/contracts/market.nim
Outdated
| _: type OnChainMarket, | ||
| contract: Marketplace, | ||
| rewardRecipient = Address.none, | ||
| requestCacheSize: uint16 = 0, |
There was a problem hiding this comment.
Maybe we don't need a default value here since there is a default value provided from the config?
There was a problem hiding this comment.
Another option is you could set a default value in the constructor and in the config similar to how it's being done with DefaultBlockTtl. This would remove the need to pass in a value to the constructor in the tests.
There was a problem hiding this comment.
I like the DefaultBlockTtl solution. I applied it.
codex/conf.nim
Outdated
| "The size of the request cache - " & | ||
| "reduce the contract calls to get the request data.", |
There was a problem hiding this comment.
| "The size of the request cache - " & | |
| "reduce the contract calls to get the request data.", | |
| "Maximum number of StorageRequests kept in memory. " & | |
| "Reduces fetching of StorageRequest data from the contract.", |
Okay I will check your suggestions. It is still in draft because you mentioned in #1083 that it would be good to include calls to nim-codex/codex/contracts/market.nim Lines 46 to 52 in 58a962a Am I missing something ? |
codex/conf.nim
Outdated
| marketplaceRequestCacheSize* {. | ||
| desc: | ||
| "Maximum number of StorageRequests kept in memory." & | ||
| "Reduces fetching of StorageRequest data from the contract.", | ||
| defaultValue: DefaultRequestCacheSize, | ||
| defaultValueDesc: $DefaultRequestCacheSize, | ||
| name: "request-cache-size" | ||
| .}: uint16 |
There was a problem hiding this comment.
I am not sure if this really needs to be CLI flag 😅 IMHO this only will confuse users who do not need this sort of granular control over the node. I would either remove this, or make this flag hidden, which will make it not show on CLI help page and maybe add note to our documentation. Which btw. should be updated anyhow as you are adding new CLI flag - the configuration section. aa
There was a problem hiding this comment.
I think we need to keep it because we would be in a situation where we are trying to debug something and we may want to change the value without building a new version. But I agree with you that it would confuse the users, so I think the best solution is to make it hidden.
For the documentation, yes I will update it after the PR is merged.
Fix #1089