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

feat: Allows locking multiple denoms #61

Merged
merged 7 commits into from
Aug 14, 2024
Merged

Conversation

p-offtermatt
Copy link
Member

@p-offtermatt p-offtermatt commented Aug 13, 2024

This PR includes these changes:

  • Add the max_validator_shares_participating parameter, which will eventually determine which validators tokenized shares Hydro will accept. The actual logic for using the parameter is not in this PR yet
  • Changes the denom validation: Instead of just accepting stATOM, we now accept all denoms that pass a "validate_denom" function. This function accepts only tokenized LSM share tokens of validators that are currently in the validator set, and it will also return the validator whose shares are represented by the given denom. The function is just implemented as a skeleton so far, the actual logic will come later, as it relies on querying the chain.
  • Changes the unlock function to send back all different locked denoms, and in the correct amounts

@p-offtermatt p-offtermatt marked this pull request as ready for review August 13, 2024 07:44
contracts/hydro/src/lsm_integration.rs Show resolved Hide resolved
amount: lock_entry.funds.amount,
};

response = response.add_message(BankMsg::Send {
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess the performance would be better if we had only one BankMsg::Send with multiple tokens?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if it makes any difference, since this is processed by the wasm vm anyways, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

It is actually handled by the x/wasm module that needs to dispatch each individual message to the proper cosmos module, which then handles it. So current code means that send() will be handled N times by the x/bank module. But we can keep it like this for now.

contracts/hydro/src/lsm_integration.rs Outdated Show resolved Hide resolved
@dusan-maksimovic
Copy link
Contributor

Left some comments, but otherwise LGTM. We will make some changes to validate_denom() and get_validator_from_denom() to make them work with proper IBC denoms validation. I will submit a PR that handles it after we merge this one.

@p-offtermatt
Copy link
Member Author

Perfect, thanks. Yeah, this PR has some placeholder implementations right now that I anticipate we'll eventually work together to make real :)

@dusan-maksimovic dusan-maksimovic self-requested a review August 14, 2024 12:11
@p-offtermatt p-offtermatt merged commit a636c9d into main Aug 14, 2024
4 checks passed
@p-offtermatt p-offtermatt deleted the ph/lock-multiple-denoms branch September 24, 2024 10:42
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.

2 participants