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

Bank Module Send Disabled Token Configuration #11859

Closed
4 tasks
iramiller opened this issue May 3, 2022 · 5 comments · Fixed by #11981
Closed
4 tasks

Bank Module Send Disabled Token Configuration #11859

iramiller opened this issue May 3, 2022 · 5 comments · Fixed by #11981

Comments

@iramiller
Copy link
Contributor

Summary of Bug

The bank module stores the Send Disabled status for coins in a configuration block. This approach does not scale to a large number of token denominations. As additional coins are added to this parameter configuration the gas read/write costs for maintaining the list becomes excessive.

Version

latest/main

Steps to Reproduce

The current implementation of the Send method checks parameter configuration on every call to determine if a coin is allowed to be sent.

// IsSendEnabledCoin returns the current SendEnabled status of the provided coin's denom
func (k BaseSendKeeper) IsSendEnabledCoin(ctx sdk.Context, coin sdk.Coin) bool {
return k.GetParams(ctx).SendEnabledDenom(coin.Denom)
}

func (p Params) SendEnabledDenom(denom string) bool {
for _, pse := range p.SendEnabled {
if pse.Denom == denom {
return pse.Enabled
}
}
return p.DefaultSendEnabled
}

This approach is extremely inefficient and does not scale to a large number of restricted coins. Within the Provenance Blockchain there are many coin denominations which are set to "send disabled" so that users can not transfer these coins directly and must use appropriate APIs to do so.

A more efficient approach would be to store the send disabled status directly in the KVStore using a key based on the denomination. This would remove a significant amount of processing from each send call as well as allow a much larger number of tokens to be restricted/send disabled without impacting transaction performance.


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@alexanderbez
Copy link
Contributor

Hey @iramiller. Thanks for opening up the issue.

A more efficient approach would be to store the send disabled status directly in the KVStore using a key based on the denomination.

This is essentially how it already works. The main difference the param blob is stored as a single object, rather than individual KV records. So yes, the gas costs would increase proportionally to the number of denoms your chain supports.

I think having these as direct values in the x/bank store could make sense. I'm all for the proposal, but I'm just trying to get the team better about prioritizing and creating roadmaps.

@marbar3778 are you in favor of such a change? I'd be happy to tackle it.

@iramiller
Copy link
Contributor Author

So yes, the gas costs would increase proportionally to the number of denoms your chain supports.

For reference we are exceeding our 4M gas per tx limits when working with this data structure on our testnet. Granted our testnet has a lot of extraneous data but I expect our mainnet will get there eventually. Our network is a unique case because we model securities on it and those require a smart contract facilitated transfer to move between accounts--thus the send disabled flag settings.

cc: @dwedul-figure who also has some thoughts on this and was looking at how we might need to solve this using our fork.

@alexanderbez
Copy link
Contributor

alexanderbez commented May 3, 2022

Yes, totally makes sense. The proposed solution is trivial and makes sense. I just want buy-in from a few other folks before I start working on it.

@alexanderbez alexanderbez self-assigned this May 3, 2022
@dwedul-figure
Copy link
Collaborator

I can also pick this up if you'd like. Just let me know.

@alexanderbez
Copy link
Contributor

@dwedul-figure go for it! Just make sure we have a migration for it as well 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants