Loans: Add Runtime API #1457
Loans: Add Runtime API #1457
Conversation
lemunozm
added
P7-asap
lemunozm
requested review from
mustermeiszer,
branan,
hieronx,
mikiquantum,
NunoAlexandre,
wischli,
cdamian and
thea-leake
as code owners
pallets/loans/src/entities/loans.rs
Outdated
| #[derive(Encode, Decode, Clone, PartialEq, Eq, RuntimeDebugNoBound, TypeInfo, MaxEncodedLen)] | ||
| #[scale_info(skip_type_params(T))] | ||
| #[cfg_attr(feature = "std", derive(serde::Serialize, serde::Deserialize))] | ||
| #[cfg_attr(feature = "std", serde(bound = ""))] | ||
| pub struct ActiveLoanInfo<T: Config> { |
There was a problem hiding this comment.
This is a bit tricky and probably useful for futures runtime APIs.
The line with bound = "" is mandatory for cases where you have both, the scale_info and derive(serde) lines. serde defaults to adding Serialize and Deserialize bounds to T, but T, in this case, does not need (and we don't want) to be serializable, it's only used to fetch information from associated types. With bounds = "", we remove those implicit bounds from serde.
There was a problem hiding this comment.
Thanks for clarifying this. So this is the serde equivalent of scale_info(skip_type_params(T) more or less?
There was a problem hiding this comment.
Yeah! More or less. Also, if there were other bounds, and you do not want to "reset them" you should add bound = "K: MyOtherBound". That way you've reset the Serialize bound to T and K but not the MyOtherBound to K.
|
@lemunozm Can you also add an integration test for this similar to the one that we have for rewards? |
|
Yeah, I wanted to do it, but looking into it, I've realized it would be quite time-consuming (creating a pool, creating assets, registering permissions, creating investors, investing in the pool, borrowing from loans to make the "active" loan, ...) just for being able to make the runtime API call, and right now I need to put efforts in other tasks for the release 😓. Also, I would need to create integration tests for all loans related things. Right now, it's well-tested at pallet level but not at runtime level. I've created an issue to track this work here: #1458 |
There was a problem hiding this comment.
LGTM apart from my minor nit and the supposedly missing #[serde(rename_all = "camelCase")] for all call input arguments based on this comment.
libs/traits/Cargo.toml
Outdated
| @@ -43,6 +44,7 @@ std = [ | |||
| "sp-std/std", | |||
| "cfg-primitives/std", | |||
| "scale-info/std", | |||
| "serde/std", | |||
There was a problem hiding this comment.
Since it's optional, shouldn't we apply ?
| "serde/std", | |
| "serde?/std", |
There was a problem hiding this comment.
Wondering why adding this broke tests?
There was a problem hiding this comment.
I think:
serde?/stdin caseserdeis enable it addsserde/stdserde/srdenablesserdeand it addsserde/std(what we want here)
lemunozm
force-pushed
the
loans/runtime-api
branch
from
25c9ab1
to
c705b89
Compare
lemunozm
force-pushed
the
loans/runtime-api
branch
from
50b1cd4
to
a68041a
Compare
|
Much much simpler PR after removing the annoying RPC part. Thanks @wischli! Ref: Slack thread |
Description
Fix partially #1174 for loans. I have not wanted to add the pool part because I'm not pretty involved in the index pool feature, and things have changed a bit from the last specs regarding this.
Also, not sure what we gain by merging pool & loan things in the same RuntimeAPI call. The user can make two API requests, and we maintain things isolated. If making two calls comes with some performance cost (that, as a first view, doesn't, because both calls can be done concurrently), then I think we can add an API wrapper that performs both API requests, keeping things isolated.
Changes and Descriptions
ActiveLoandata and computed values aspresent_valueandinterest_accrued.Integration tests=> Can be done in another PR => Loans: add integration tests #1458