-
Notifications
You must be signed in to change notification settings - Fork 11
Implementing Elys lending library #370
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
base: main
Are you sure you want to change the base?
Implementing Elys lending library #370
Conversation
WalkthroughA new Elys lending library is introduced, including contract logic, configuration, schema, and integration into the workspace and program manager. The changes add message types, configuration validation, and query helpers for Elys lending, update workspace and dependency configurations, and extend the program manager to support the new lending library type. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ProgramManager
participant ElysLendingLibrary
participant ElysChain
User->>ProgramManager: Instantiate or update program (with ValenceElysLending config)
ProgramManager->>ElysLendingLibrary: Passes config for validation/instantiation
ElysLendingLibrary->>ElysChain: Query pool info, balances, etc.
ElysChain-->>ElysLendingLibrary: Pool and account info
ElysLendingLibrary-->>ProgramManager: Success or error response
User->>ElysLendingLibrary: Execute lending action (lend/withdraw/claim)
ElysLendingLibrary->>ElysChain: Submit MsgBond/MsgUnbond/MsgClaimRewards
ElysChain-->>ElysLendingLibrary: Reply/result
ElysLendingLibrary->>User: Confirmation and/or asset transfer
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 8
🧹 Nitpick comments (13)
program-manager/src/library.rs (1)
101-101
: NewValenceElysLending
variant added toLibraryConfig
.
You’ve correctly introduced theValenceElysLending(valence_elys_lending::msg::LibraryConfig)
variant. Please verify that:
- The
manager_impl_library_configs
macro supports this new variant.- The JSON schema (
program-manager/schema/valence-program-manager.json
) is updated to include this variant for instantiation and update messages.Let me know if you need help updating the schema or macro usage.
contracts/libraries/elys-lending/.cargo/config.toml (1)
1-3
: Define Cargo aliases for build and schema generation.
Thewasm
andschema
aliases streamline the build and schema commands for the Elys lending library. Consider documenting these aliases in the project README for discoverability, but the definitions themselves are correct.contracts/libraries/elys-lending/README.md (1)
3-4
: Typo in documentation.
The word "withdrawed" should be corrected to "withdrawn" for proper grammar:- depositing the withdrawed tokens into an **output account**. + depositing the withdrawn tokens into an **output account**.contracts/libraries/elys-lending/src/bin/schema.rs (1)
8-10
: Improve readability in macro invocation.
Add a space after the comma between generic parameters to align with Rust formatting conventions:- execute: ExecuteMsg<FunctionMsgs,LibraryConfigUpdate>, + execute: ExecuteMsg<FunctionMsgs, LibraryConfigUpdate>,contracts/libraries/elys-lending/Cargo.toml (1)
1-7
: Enhance crate metadata.
Consider enriching the[package]
section withdescription
,homepage
, anddocumentation
fields to improve discoverability on crates.io and docs.rs:[package] name = "valence-elys-lending" +description = "Elys lending library for the Valence protocol" +homepage = "https://github.com/timewave-computer/valence-protocol" +documentation = "https://docs.rs/valence-elys-lending" authors.workspace = true edition.workspace = true license.workspace = true version.workspace = true repository.workspace = truepackages/lending-utils/src/elys.rs (2)
28-44
: Consider the data types used in PoolResponse.All numeric fields in
PoolResponse
(rates, ratios, totals, etc.) are defined asString
. While this might be intentional for compatibility with the Elys protocol, consider documenting why strings are used instead of numeric types likeDecimal
orUint128
. This would help future maintainers understand the design decision.
17-44
: Consider adding documentation for complex pool parameters.The structs are properly defined, but the
PoolResponse
contains many financial parameters that would benefit from documentation comments explaining their purpose and expected formats.+/// Response containing pool information pub struct QueryGetPoolResponse { pub pool: ::core::option::Option<PoolResponse>, } +/// Detailed pool parameters and state pub struct PoolResponse { + /// The denomination used for deposits in this pool pub deposit_denom: String, + /// Current redemption rate as a string-encoded decimal pub redemption_rate: String, // ... add documentation for other financial parameters pub pool_id: u64, pub total_deposit: String, pub total_borrow: String, pub borrow_ratio: String, pub max_withdraw_ratio: String, }contracts/libraries/elys-lending/src/msg.rs (2)
61-77
: Use field shorthand syntax.The validation implementation is correct, but you can simplify the struct initialization.
Ok(Config { input_addr, output_addr, - pool_id: pool_id, + pool_id, })
108-116
: Use field shorthand syntax in constructor.Config { input_addr, output_addr, - pool_id: pool_id, + pool_id, }contracts/libraries/elys-lending/schema/valence-elys-lending.json (4)
72-106
: Consider centralizing repeated definitions.
TheLibraryConfig
schema currently references local definitions forLibraryAccountType
andUint64
, which are duplicated in multiple blocks. For maintainability, consider moving shared types (e.g.,LibraryAccountType
,Uint64
) into a single, root-leveldefinitions
or$defs
section, and updating all$ref
pointers accordingly.
387-422
: LibraryConfigUpdate could use a description.
While the partial update fields are correct, consider adding a brief"description"
at theLibraryConfigUpdate
schema level to explain its purpose and how null values are handled.
423-438
: Deduplicate numeric wrapper definitions.
TheUint128
andUint64
definitions are repeated here. Consider referencing the shared definitions from earlier or moving them to a common location to avoid redundancy.
505-725
: Consolidate response definitions across schemas.
The response schemas forget_library_config
,get_processor
,get_raw_library_config
, andownership
each include local definitions (Addr
,Uint64
,LibraryAccountType
,Expiration
,Timestamp
) that are largely duplicated. For better maintainability and DRY compliance, centralize shared types in a root-level definitions section and update local$ref
references.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lock
is excluded by!**/*.lock
📒 Files selected for processing (15)
Cargo.toml
(2 hunks)contracts/libraries/elys-lending/.cargo/config.toml
(1 hunks)contracts/libraries/elys-lending/Cargo.toml
(1 hunks)contracts/libraries/elys-lending/README.md
(1 hunks)contracts/libraries/elys-lending/schema/valence-elys-lending.json
(1 hunks)contracts/libraries/elys-lending/src/bin/schema.rs
(1 hunks)contracts/libraries/elys-lending/src/contract.rs
(1 hunks)contracts/libraries/elys-lending/src/lib.rs
(1 hunks)contracts/libraries/elys-lending/src/msg.rs
(1 hunks)packages/lending-utils/Cargo.toml
(1 hunks)packages/lending-utils/src/elys.rs
(1 hunks)packages/lending-utils/src/lib.rs
(1 hunks)program-manager/Cargo.toml
(1 hunks)program-manager/schema/valence-program-manager.json
(4 hunks)program-manager/src/library.rs
(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
contracts/libraries/elys-lending/src/bin/schema.rs (1)
contracts/libraries/elys-lending/src/contract.rs (3)
instantiate
(27-34)execute
(37-44)query
(248-266)
packages/lending-utils/src/elys.rs (2)
contracts/libraries/elys-lending/src/contract.rs (1)
query
(248-266)program-manager/src/error.rs (1)
generic_err
(92-94)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: setup-local-ic
- GitHub Check: solidity contracts
- GitHub Check: build-and-deploy
🔇 Additional comments (25)
packages/lending-utils/Cargo.toml (1)
11-13
: Dependencies added for Elys integration look correct.
Theserde
,schemars
, andprost
crates are appropriately introduced for JSON serialization, schema generation, and protobuf support in the newelys
module. Using workspace versions forserde
andschemars
maintains consistency.packages/lending-utils/src/lib.rs (1)
1-1
: Expose the newelys
module.
Addingpub mod elys;
correctly makes theelys
helper functions available in thelending-utils
crate. Ensure thatpackages/lending-utils/src/elys.rs
fully implements the necessary CosmWasm message and query types.program-manager/Cargo.toml (1)
48-48
: Addvalence-elys-lending
to workspace dependencies.
The new dependency aligns with othervalence-*
libraries and enables the program manager to build and reference the Elys lending library. Version specification as{ workspace = true }
matches the workspace pattern.contracts/libraries/elys-lending/src/lib.rs (1)
1-2
: Public module declarations look good.
Thelib.rs
correctly exposes thecontract
andmsg
modules, laying out the library’s core structure. Ensure thatcontract.rs
andmsg.rs
include proper documentation comments for public types and functions to aid discoverability.Cargo.toml (1)
102-103
: Integrate new Elys lending library.
Thevalence-elys-lending
dependency with the"library"
feature is correctly added under[workspace.dependencies]
. Verify that the pathcontracts/libraries/elys-lending
is accurate and that the workspace build picks up this crate as expected.packages/lending-utils/src/elys.rs (4)
6-16
: LGTM!The
ElysQuery
enum is properly structured with appropriate derives and implements the requiredCustomQuery
trait.
55-109
: Protobuf messages are well-defined.The message structs are properly configured for protobuf serialization with appropriate prost attributes and serde aliases for field name compatibility.
1-4
: LGTM! Clear documentation of compatibility rationale.The file header effectively explains why these messages are being redefined for CosmWasm 2.x compatibility and references the source version.
6-15
: LGTM! Proper CustomQuery implementation.The
ElysQuery
enum is well-defined with appropriate derives and implements theCustomQuery
trait correctly. The snake_case serde annotation ensures consistent JSON serialization.program-manager/schema/valence-program-manager.json (1)
929-941
: Schema additions are consistent and well-structured.The new
ValenceElysLending
library configuration follows the established pattern with appropriate required fields for instantiation and optional fields for updates.Also applies to: 1069-1103, 1500-1512, 1713-1748
contracts/libraries/elys-lending/src/msg.rs (3)
9-24
: Message enums are well-defined.The
FunctionMsgs
enum properly covers lending operations with clear documentation, andQueryMsg
is correctly prepared for future query implementations.
26-59
: LibraryConfig is properly structured.The configuration struct has clear field documentation and appropriate validation logic for converting account types to addresses.
79-99
: Update logic includes proper validation.Good practice to validate the pool exists before updating the configuration. The update method correctly handles partial updates.
contracts/libraries/elys-lending/src/contract.rs (6)
1-25
: Contract setup follows best practices.Good use of constants for reply IDs and standard contract versioning from Cargo.toml.
26-44
: Entry points properly delegate to base library.The instantiate and execute functions correctly use the base library pattern with appropriate type parameters.
54-88
: Lend implementation is correct.The lending logic properly validates balance, constructs the bond message, and executes on behalf of the input address. The use of Stargate messages is appropriate for custom Cosmos SDK messages.
98-107
: Verify the single locked coin assumption.The code assumes only one coin will be locked for this pool, but takes the first coin without checking the denomination. This could lead to withdrawing the wrong asset if multiple coins are locked.
Consider adding validation to ensure the locked coin matches the expected denomination:
+ // Query the pool to get the expected deposit denom + let pool = valence_lending_utils::elys::query_pool(&deps, cfg.pool_id.u64())?; + let balance_locked: Coin = tokens_locked .locked_committed .into_iter() - .next() // Take the first coin + .find(|coin| coin.denom == pool.deposit_denom) .ok_or_else(|| { LibraryError::ExecutionError( "No funds locked for the specified denom".to_string(), ) })?;
175-236
: Reply handlers are well-implemented.The reply logic correctly handles post-execution transfers. Note that the
CLAIM_REPLY_ID
handler is currently unreachable due to the missing submessage in the ClaimRewards function.
238-266
: Configuration and query functions follow standard patterns.The update and query implementations correctly delegate to appropriate handlers and return expected responses.
contracts/libraries/elys-lending/schema/valence-elys-lending.json (6)
2-4
: Verify version consistency with contract code.
Thecontract_version
("0.2.0") andidl_version
("1.0.0") should align with the version in your Cargo.toml, the schema generation script, and the Elys lending library code.Can you confirm that
0.2.0
matches the current crate version and that the IDL version is correct?
5-24
: Instantiate message schema looks correct.
TheInstantiateMsg
defines all required fields (config
,owner
,processor
) with strictadditionalProperties: false
, and includes a valid draft-07$schema
.
107-111
: Uint64 wrapper definition is solid.
TheUint64
type uses a string encoding with clear documentation and examples.
113-183
: Execute message top-level variants are well-defined.
TheExecuteMsg
oneOf block cleanly separatesprocess_function
,update_config
,update_processor
, andupdate_ownership
variants, each with strict property checks andadditionalProperties: false
.
186-283
: Action and Expiration definitions are comprehensive.
TheAction
union covers object and string-based ownership operations, and theExpiration
oneOf variants correctly model block height, time, and never-expire cases.
441-500
: Query message schema is clear and strict.
TheQueryMsg
oneOf variants forget_processor
,get_library_config
,get_raw_library_config
, andownership
are correctly defined with no additional properties.
pub fn query_pool(deps: &DepsMut<ElysQuery>, pool_id: u64) -> StdResult<PoolResponse> { | ||
let query = ElysQuery::QueryGetPool { pool_id }; | ||
let query_pool_response: Option<QueryGetPoolResponse> = deps.querier.query(&query.into()).ok(); | ||
|
||
// Check if pool exists, otherwise return an error | ||
let pool_response = | ||
query_pool_response.ok_or_else(|| StdError::generic_err("failed to query pool"))?; | ||
let pool = pool_response | ||
.pool | ||
.ok_or_else(|| StdError::generic_err("pool not found"))?; | ||
|
||
Ok(pool) | ||
} |
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.
🛠️ Refactor suggestion
Improve error handling to preserve error details.
The current implementation swallows error details and provides generic error messages. This makes debugging difficult when queries fail.
Consider preserving the original error information:
pub fn query_pool(deps: &DepsMut<ElysQuery>, pool_id: u64) -> StdResult<PoolResponse> {
let query = ElysQuery::QueryGetPool { pool_id };
- let query_pool_response: Option<QueryGetPoolResponse> = deps.querier.query(&query.into()).ok();
+ let query_pool_response: QueryGetPoolResponse = deps.querier.query(&query.into())
+ .map_err(|e| StdError::generic_err(format!("Failed to query pool {}: {}", pool_id, e)))?;
- // Check if pool exists, otherwise return an error
- let pool_response =
- query_pool_response.ok_or_else(|| StdError::generic_err("failed to query pool"))?;
- let pool = pool_response
+ let pool = query_pool_response
.pool
- .ok_or_else(|| StdError::generic_err("pool not found"))?;
+ .ok_or_else(|| StdError::generic_err(format!("Pool {} not found", pool_id)))?;
Ok(pool)
}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
pub fn query_pool(deps: &DepsMut<ElysQuery>, pool_id: u64) -> StdResult<PoolResponse> { | |
let query = ElysQuery::QueryGetPool { pool_id }; | |
let query_pool_response: Option<QueryGetPoolResponse> = deps.querier.query(&query.into()).ok(); | |
// Check if pool exists, otherwise return an error | |
let pool_response = | |
query_pool_response.ok_or_else(|| StdError::generic_err("failed to query pool"))?; | |
let pool = pool_response | |
.pool | |
.ok_or_else(|| StdError::generic_err("pool not found"))?; | |
Ok(pool) | |
} | |
pub fn query_pool(deps: &DepsMut<ElysQuery>, pool_id: u64) -> StdResult<PoolResponse> { | |
let query = ElysQuery::QueryGetPool { pool_id }; | |
let query_pool_response: QueryGetPoolResponse = deps.querier.query(&query.into()) | |
.map_err(|e| StdError::generic_err(format!("Failed to query pool {}: {}", pool_id, e)))?; | |
let pool = query_pool_response | |
.pool | |
.ok_or_else(|| StdError::generic_err(format!("Pool {} not found", pool_id)))?; | |
Ok(pool) | |
} |
🤖 Prompt for AI Agents
In packages/lending-utils/src/elys.rs around lines 111 to 123, the error
handling in query_pool currently replaces original errors with generic messages,
losing valuable debugging information. Modify the code to capture the original
error from the query call and propagate it or include its details in the
returned StdError. This can be done by using map_err or a similar method to
convert the original error into a StdError while preserving the original error
message instead of using generic_err with a fixed string.
🛠️ Refactor suggestion
Improve error handling and function signature.
The function has several issues that should be addressed:
- Uses
&DepsMut
but only reads data - should use&Deps
- Swallows actual error information with
.ok()
- Generic error messages aren't helpful for debugging
-pub fn query_pool(deps: &DepsMut<ElysQuery>, pool_id: u64) -> StdResult<PoolResponse> {
+/// Queries pool information by pool ID from the Elys network
+pub fn query_pool(deps: &Deps<ElysQuery>, pool_id: u64) -> StdResult<PoolResponse> {
let query = ElysQuery::QueryGetPool { pool_id };
- let query_pool_response: Option<QueryGetPoolResponse> = deps.querier.query(&query.into()).ok();
+ let query_pool_response: QueryGetPoolResponse = deps.querier.query(&query.into())
+ .map_err(|e| StdError::generic_err(format!("Failed to query pool {}: {}", pool_id, e)))?;
- // Check if pool exists, otherwise return an error
- let pool_response =
- query_pool_response.ok_or_else(|| StdError::generic_err("failed to query pool"))?;
- let pool = pool_response
+ let pool = query_pool_response
.pool
- .ok_or_else(|| StdError::generic_err("pool not found"))?;
+ .ok_or_else(|| StdError::generic_err(format!("Pool {} not found", pool_id)))?;
Ok(pool)
}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
pub fn query_pool(deps: &DepsMut<ElysQuery>, pool_id: u64) -> StdResult<PoolResponse> { | |
let query = ElysQuery::QueryGetPool { pool_id }; | |
let query_pool_response: Option<QueryGetPoolResponse> = deps.querier.query(&query.into()).ok(); | |
// Check if pool exists, otherwise return an error | |
let pool_response = | |
query_pool_response.ok_or_else(|| StdError::generic_err("failed to query pool"))?; | |
let pool = pool_response | |
.pool | |
.ok_or_else(|| StdError::generic_err("pool not found"))?; | |
Ok(pool) | |
} | |
/// Queries pool information by pool ID from the Elys network | |
pub fn query_pool(deps: &Deps<ElysQuery>, pool_id: u64) -> StdResult<PoolResponse> { | |
let query = ElysQuery::QueryGetPool { pool_id }; | |
let query_pool_response: QueryGetPoolResponse = deps | |
.querier | |
.query(&query.into()) | |
.map_err(|e| StdError::generic_err(format!("Failed to query pool {}: {}", pool_id, e)))?; | |
let pool = query_pool_response | |
.pool | |
.ok_or_else(|| StdError::generic_err(format!("Pool {} not found", pool_id)))?; | |
Ok(pool) | |
} |
🤖 Prompt for AI Agents
In packages/lending-utils/src/elys.rs around lines 111 to 123, the function
query_pool uses &DepsMut when it only reads data, swallows error details by
using .ok(), and returns generic error messages. Change the function signature
to accept &Deps instead of &DepsMut, replace .ok() with proper error propagation
using the ? operator to preserve error details, and update error messages to
include specific context from the original errors to improve debugging.
"LibraryAccountType": { | ||
"description": "A helper type that is used to associate an account or library with an id When a program is not instantiated yet, ids will be used to reference accounts and libraries When a program is instantiated, the ids will be replaced by the instantiated addresses", | ||
"oneOf": [ | ||
{ | ||
"type": "object", | ||
"required": [ | ||
"|library_account_addr|" | ||
], | ||
"properties": { | ||
"|library_account_addr|": { | ||
"type": "string" | ||
} | ||
}, | ||
"additionalProperties": false | ||
}, | ||
{ | ||
"type": "object", | ||
"required": [ | ||
"|account_id|" | ||
], | ||
"properties": { | ||
"|account_id|": { | ||
"type": "integer", | ||
"format": "uint64", | ||
"minimum": 0.0 | ||
} | ||
}, | ||
"additionalProperties": false | ||
}, | ||
{ | ||
"type": "object", | ||
"required": [ | ||
"|library_id|" | ||
], | ||
"properties": { | ||
"|library_id|": { | ||
"type": "integer", | ||
"format": "uint64", | ||
"minimum": 0.0 | ||
} | ||
}, | ||
"additionalProperties": false | ||
} | ||
] |
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.
Critical: Placeholder keys in LibraryAccountType
need replacement.
The LibraryAccountType
definition is using placeholder property names like |library_account_addr|
, |account_id|
, and |library_id|
. These must be replaced with the actual field names (e.g., "library_account_addr"
, "account_id"
, "library_id"
) for the schema to validate correctly.
🤖 Prompt for AI Agents
In contracts/libraries/elys-lending/schema/valence-elys-lending.json between
lines 27 and 70, replace the placeholder keys surrounded by pipes such as
"|library_account_addr|", "|account_id|", and "|library_id|" with the actual
field names without pipes, like "library_account_addr", "account_id", and
"library_id". This change is necessary to ensure the JSON schema validates
correctly.
"LibraryAccountType": { | ||
"description": "A helper type that is used to associate an account or library with an id When a program is not instantiated yet, ids will be used to reference accounts and libraries When a program is instantiated, the ids will be replaced by the instantiated addresses", | ||
"oneOf": [ | ||
{ | ||
"type": "object", | ||
"required": [ | ||
"|library_account_addr|" | ||
], | ||
"properties": { | ||
"|library_account_addr|": { | ||
"type": "string" | ||
} | ||
}, | ||
"additionalProperties": false | ||
}, | ||
{ | ||
"type": "object", | ||
"required": [ | ||
"|account_id|" | ||
], | ||
"properties": { | ||
"|account_id|": { | ||
"type": "integer", | ||
"format": "uint64", | ||
"minimum": 0.0 | ||
} | ||
}, | ||
"additionalProperties": false | ||
}, | ||
{ | ||
"type": "object", | ||
"required": [ | ||
"|library_id|" | ||
], | ||
"properties": { | ||
"|library_id|": { | ||
"type": "integer", | ||
"format": "uint64", | ||
"minimum": 0.0 | ||
} | ||
}, | ||
"additionalProperties": false | ||
} | ||
] | ||
}, |
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.
Critical: Placeholder keys repeated in second LibraryAccountType
.
The duplicate LibraryAccountType
under FunctionMsgs
also contains |library_account_addr|
, |account_id|
, and |library_id|
placeholders. Please replace these with actual field names or adjust generation tooling.
🤖 Prompt for AI Agents
In contracts/libraries/elys-lending/schema/valence-elys-lending.json between
lines 342 and 386, the placeholder keys like "|library_account_addr|",
"|account_id|", and "|library_id|" are repeated in the second LibraryAccountType
under FunctionMsgs. Replace these placeholder keys with actual, meaningful field
names that reflect their purpose or adjust the schema generation tooling to
output correct field names instead of placeholders.
20a96d9
to
5867c48
Compare
5867c48
to
a6a2468
Compare
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.
Actionable comments posted: 0
🧹 Nitpick comments (5)
docs/src/libraries/cosmwasm/elys_lending.md (5)
1-1
: Title-case the main heading.The top-level heading should use title case for consistency.
Apply this diff:-# Valence Elys Lending library +# Valence Elys Lending Library
5-5
: Hyphenate the compound adjective.“High Level Flow” should be “High-Level Flow” to correctly form the compound adjective.
-## High Level Flow +## High-Level Flow🧰 Tools
🪛 LanguageTool
[uncategorized] ~5-~5: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...ns directly from the input account. ## High Level Flow ```mermaid --- title: Elys Lendin...(EN_COMPOUND_ADJECTIVE_INTERNAL)
19-23
: Unify the naming of the claim operation.In the mermaid diagram you use “Claim” in step 1 and “Claim Rewards” in step 5, but the function name is
ClaimRewards
. Consider renaming both toClaimRewards
for consistency with the rest of the docs and codebase.
39-48
: Show derive macros in the config snippet.The actual
LibraryConfig
struct in Rust likely derives serialization, schema, and debug traits. Reflect that in the snippet for accuracy:```rust +#[derive(Serialize, Deserialize, Clone, Debug, PartialEq, JsonSchema)] pub struct LibraryConfig { /// Address of the input account pub input_addr: LibraryAccountType, /// Address of the output account pub output_addr: LibraryAccountType, /// ID of the pool we are going to lend to pub pool_id: Uint64, }
--- `37-38`: **Link key types to their definitions.** To improve discoverability, link `LibraryConfig`, `LibraryAccountType`, and `Uint64` to their code definitions or API docs. E.g.: > The library is configured on instantiation via the [`LibraryConfig`](#LibraryConfig) type. </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** **Plan: Pro** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between a6a24682f8fe9c2449a85c2e52ea1e1954822e6e and 077bd9d7d361eaeee4156d13046b7e0684e5ee70. </details> <details> <summary>📒 Files selected for processing (2)</summary> * `docs/src/SUMMARY.md` (1 hunks) * `docs/src/libraries/cosmwasm/elys_lending.md` (1 hunks) </details> <details> <summary>✅ Files skipped from review due to trivial changes (1)</summary> * docs/src/SUMMARY.md </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 LanguageTool</summary> <details> <summary>docs/src/libraries/cosmwasm/elys_lending.md</summary> [uncategorized] ~5-~5: If this is a compound adjective that modifies the following noun, use a hyphen. Context: ...ns directly from the input account. ## High Level Flow ```mermaid --- title: Elys Lendin... (EN_COMPOUND_ADJECTIVE_INTERNAL) --- [grammar] ~60-~60: Using ‘plenty’ without ‘of’ is considered to be informal. Context: ...n the vault for the input account. 2. **Amount Calculation**: Uses the exact amount if specified; ... (PLENTY_OF_NOUNS) </details> </details> </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
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 wait to merge until tested on Elys, hopefully soon.
Only have a question regarding rewards as normally for most protocols the rewards are automatically claimed when unbonding, is this not the case for Elys?
#[cw_serde] | ||
pub enum FunctionMsgs { | ||
/// Message to lend tokens. | ||
Lend {}, | ||
/// Message to withdraw tokens. If amount is not specified, full amount will be withdrawn. | ||
Withdraw { amount: Option<Uint128> }, | ||
/// Message to claim reward tokens. | ||
ClaimRewards {}, | ||
} |
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.
Are rewards not automatically claimed when unbonding?
Depending on the case I see a possible improvement that can be done here:
- If not automatically claimed, when we withdraw, we also claim rewards and send them to the output account.
- If automatically claimed, when we withdraw we need to send the rewards claimed as well otherwise they will be stuck in the input account
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.
Unfortunately, the rewards aren’t claimed automatically, although they mentioned in the Telegram group that they would fix it so they get claimed by default. I could ask if that has been resolved.
Either way, I agree that it would be nicer if claiming happened automatically when withdrawing. I’m just not sure whether it’s still better to keep the claim as a separate function or remove it entirely?
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.
Hm, I can see maybe we want to keep it in case we want to claim rewards and keep lending? I'm fine with keeping it since you already wrote it.
Yeah if they are going to change claim rewards we probably have to wait until they change it and then adjust the library to send the claimed rewards on withdraw.
This PR adds implementation for Elys lending library.
*NOTE: since the contracts are not yet deployed on Elys network (permissioned cosmwasm), the library is not tested.
Summary by CodeRabbit
New Features
Documentation
Chores