Skip to content
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

Add logstoredinterests #1429

Merged
merged 4 commits into from
Aug 30, 2022
Merged

Add logstoredinterests #1429

merged 4 commits into from
Aug 30, 2022

Conversation

prasannavl
Copy link
Member

/kind feature

  • To list all interest rates and loans in vaults


auto height = ::ChainActive().Height();
using VaultInfo = std::tuple<DCT_ID,CAmount,CInterestRateV3>;
std::map<std::string, std::vector<VaultInfo>> items;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not add directly to UniValue and avoid iterating again after?

Copy link
Member Author

@prasannavl prasannavl Aug 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless there is a specific reason to do so, I'd highly recommend against doing that. The extra memory by the raw dataset, compared to UniValue is usually small. And so is the additional computation that they are irrelevant. Code clarity, reusability and maintenance is usually far more important to focus on. This decouples the formatting from the actual logic.

In other words, now the logic is unit-testable, apart from the formatting.

Second, the format could something else that's not JSON at all (GRPC, for instance). Mixing it up with UniValue makes things messy.

Performance aspects if it ever is a concern (unlikely in these cases), having clear separation of concerns also provides opportunities to parallelize the logic, etc. if needed. Getting UniValue will make that messy as well.

@prasannavl prasannavl merged commit 7a90326 into master Aug 30, 2022
@prasannavl prasannavl deleted the feature/logstoredinterests branch August 30, 2022 08:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants