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.

Sign up for GitHub

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

refactor(core-token-vesting-v2)!: remove recipient #139

Merged
merged 3 commits into from
Mar 11, 2024
Merged

refactor(core-token-vesting-v2)!: remove recipient #139

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4cf78a9
refactor: remove recipient
k-yang Mar 11, 2024
a9f5793
chore: bump package version
k-yang Mar 11, 2024
cde2198
refactor: remove denom field in Claim msg
k-yang Mar 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion contracts/core-token-vesting-v2/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "core-token-vesting-v2"
version = "2.0.0"
version = "2.1.0"
edition = "2021"
description = "Token vesting contract v2"

Expand Down
39 changes: 12 additions & 27 deletions contracts/core-token-vesting-v2/src/contract.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,13 +77,8 @@ pub fn execute(
ExecuteMsg::DeregisterVestingAccounts { addresses } => {
deregister_vesting_accounts(deps, env, info, addresses)
}
ExecuteMsg::Claim {
denoms: _denoms,
recipient,
} => claim(deps, env, info, recipient),
ExecuteMsg::Withdraw { amount, recipient } => {
withdraw(deps, env, info, amount, recipient)
}
ExecuteMsg::Claim { denoms: _denoms } => claim(deps, env, info),
Copy link
Contributor

Choose a reason for hiding this comment

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

If we do breaking changes for the interface we can get rid of the denom parameter also

Copy link
Member Author

Choose a reason for hiding this comment

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

Removing denoms is a breaking change for the UI because it's a mandatory field, but removing recipient is not because it's an Optional field, meaning the client doesn't have to send it.

Copy link
Contributor

Choose a reason for hiding this comment

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

we can leave it then

ExecuteMsg::Withdraw { amount } => withdraw(deps, env, info, amount),
}
}

Expand All @@ -95,15 +90,15 @@ pub fn withdraw(
_env: Env,
info: MessageInfo,
amount: Uint128,
recipient: String,
) -> Result<Response, ContractError> {
let whitelist = WHITELIST.load(deps.storage)?;
let mut unallocated_amount = UNALLOCATED_AMOUNT.load(deps.storage)?;
let denom = DENOM.load(deps.storage)?;

if !whitelist.is_admin(info.sender) {
if !whitelist.is_admin(&info.sender) {
return Err(StdError::generic_err("Unauthorized").into());
}
let recipient = info.sender.as_str();

let amount_max = min(amount, unallocated_amount);
if amount_max.is_zero() {
Expand All @@ -113,13 +108,10 @@ pub fn withdraw(
unallocated_amount -= amount_max;
UNALLOCATED_AMOUNT.save(deps.storage, &unallocated_amount)?;

// validate recipient address
deps.api.addr_validate(&recipient)?;

Ok(Response::new()
.add_messages(vec![build_send_msg(&denom, amount_max, &recipient)])
.add_messages(vec![build_send_msg(&denom, amount_max, recipient)])
.add_attribute("action", "withdraw")
.add_attribute("recipient", &recipient)
.add_attribute("recipient", recipient)
.add_attribute("amount", amount_max.to_string())
.add_attribute("unallocated_amount", unallocated_amount.to_string()))
}
Expand Down Expand Up @@ -320,12 +312,7 @@ fn deregister_vesting_account(

// transfer already vested amount to the user
let claimable_amount = vested_amount.checked_sub(claimed_amount)?;
send_if_amount_is_not_zero(
messages,
claimable_amount,
&denom,
address,
)?;
send_if_amount_is_not_zero(messages, claimable_amount, &denom, address)?;

// transfer left vesting amount to the admin
let left_vesting_amount =
Expand Down Expand Up @@ -366,16 +353,14 @@ fn claim(
deps: DepsMut,
env: Env,
info: MessageInfo,
recipient: Option<String>,
) -> Result<Response, ContractError> {
let sender = &info.sender;
let recipient = &recipient.unwrap_or_else(|| sender.to_string());
let recipient = info.sender.as_str();
let denom = DENOM.load(deps.storage)?;

let mut attrs: Vec<Attribute> = vec![];

// vesting_account existence check
let account = VESTING_ACCOUNTS.may_load(deps.storage, sender.as_str())?;
let account = VESTING_ACCOUNTS.may_load(deps.storage, recipient)?;
if account.is_none() {
return Err(StdError::generic_err(format!(
"vesting entry is not found for denom {}",
Expand All @@ -395,9 +380,9 @@ fn claim(

account.claimed_amount = vested_amount;
if account.claimed_amount == account.vesting_amount {
VESTING_ACCOUNTS.remove(deps.storage, sender.as_str());
VESTING_ACCOUNTS.remove(deps.storage, recipient);
} else {
VESTING_ACCOUNTS.save(deps.storage, sender.as_str(), &account)?;
VESTING_ACCOUNTS.save(deps.storage, recipient, &account)?;
}

attrs.extend(
Expand All @@ -412,7 +397,7 @@ fn claim(

Ok(Response::new()
.add_messages(vec![build_send_msg(&denom, claimable_amount, recipient)])
.add_attributes(vec![("action", "claim"), ("address", sender.as_str())])
.add_attributes(vec![("action", "claim"), ("address", recipient)])
.add_attributes(attrs))
}

Expand Down
2 changes: 0 additions & 2 deletions contracts/core-token-vesting-v2/src/msg.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,13 +35,11 @@ pub enum ExecuteMsg {
/// Claim is an operation that allows one to claim vested tokens.
Claim {
denoms: Vec<Denom>,
recipient: Option<String>,
},

// Withdraw allows the admin to withdraw the funds from the contract
Withdraw {
amount: Uint128,
recipient: String,
},
}

Expand Down
4 changes: 0 additions & 4 deletions contracts/core-token-vesting-v2/src/testing.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -498,7 +498,6 @@ fn test_withdraw() -> TestResult {

// unauthorized sender
let msg = ExecuteMsg::Withdraw {
recipient: "addr0000".to_string(),
amount: Uint128::new(1000),
};
require_error(
Comment on lines 498 to 503
Copy link
Contributor

Choose a reason for hiding this comment

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

📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [429-465]

The test test_withdraw correctly reflects the changes made to the ExecuteMsg::Withdraw message by removing the recipient parameter. However, it's important to ensure that the test cases cover scenarios where the sender is implicitly the recipient of the withdrawal. This is a key part of the PR's objectives, and the tests should explicitly verify that the funds are indeed sent to the info.sender address.

Additionally, the test includes a scenario where an unauthorized sender attempts to withdraw funds, which is good for verifying access control. However, it would be beneficial to add a test case that verifies the correct behavior when the sender attempts to withdraw an amount greater than their unallocated balance, ensuring that the contract logic correctly handles such cases.

Consider adding explicit assertions to verify that the withdrawn funds are sent to the info.sender address, as this is a critical aspect of the PR's objectives. Also, ensure that edge cases, such as attempting to withdraw more than the available unallocated balance, are adequately tested.

📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [467-567]

The claim_native test function has been updated to reflect the removal of the recipient parameter from the ExecuteMsg::Claim message. This test function is crucial for verifying that the claim functionality works as expected, particularly that the sender (who is also the claimant in this context) receives the claimed funds.

While the test does cover scenarios where the claim is made at different times to verify the amount claimable based on the vesting schedule, it would be beneficial to explicitly assert that the funds are sent to the info.sender address. Given the PR's objectives, ensuring that the claim functionality defaults the recipient to the sender is essential.

Enhance the claim_native test by adding explicit assertions to confirm that the claimed funds are sent to the info.sender address. This will ensure that the test suite fully covers the changes made to the claim functionality as per the PR's objectives.

Expand All @@ -511,7 +510,6 @@ fn test_withdraw() -> TestResult {

// withdraw more than unallocated
let msg = ExecuteMsg::Withdraw {
recipient: "addr0000".to_string(),
amount: Uint128::new(1001),
};
let res =
Expand Down Expand Up @@ -541,7 +539,6 @@ fn test_withdraw() -> TestResult {

// withdraw but there's no more unallocated
let msg = ExecuteMsg::Withdraw {
recipient: "addr0000".to_string(),
amount: Uint128::new(1),
};
require_error(
Expand Down Expand Up @@ -797,7 +794,6 @@ fn claim_native() -> TestResult {
// valid claim
let info = mock_info("addr0001", &[]);
let msg = ExecuteMsg::Claim {
recipient: None,
denoms: vec![],
};

Expand Down
Loading