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

reject bank/MsgSend with fees > send value #3689

Closed
rigelrozanski opened this issue Feb 20, 2019 · 9 comments
Closed

reject bank/MsgSend with fees > send value #3689

rigelrozanski opened this issue Feb 20, 2019 · 9 comments

Comments

@rigelrozanski
Copy link
Contributor

https://etherscan.io/tx/0x1f73b43dc9c48cc131a931fac7095de9e5eba0c5184ec0c5c5f1f32efa2a6bab

seems like a this kind of safeguard could reduce a lot of anxiety... something we should all probably be aiming to accomplish.

CC @sunnya97

@alexanderbez
Copy link
Contributor

What is your proposal here @rigelrozanski? To limit the fee based on some threshold relative the the sending amount? Wouldn't this ultimately depend on the tx type?

@rigelrozanski
Copy link
Contributor Author

  • fees cannot be greater than the send amount
  • ONLY apply this to bank/MsgSend

I don't think there is an appropriate generic way to perform this check - ultimately every message type would need to have it's own ruleset as to what is appropriate -

@rigelrozanski rigelrozanski changed the title reject send tx with fees > send value reject bank/MsgSend with fees > send value Feb 20, 2019
@gamarin2
Copy link
Contributor

The problem of inadvertently sending too much fees is not specific to the bank / MsgSend. I think this #3653 is a better way to address the issue.

@alexanderbez
Copy link
Contributor

Indeed, both proposals are great and I think we should certainly perform #3653

@rigelrozanski
Copy link
Contributor Author

no question that #3653 is a good idea. It helps this issue for anyone making a transaction with the CLI - however I nonetheless think that we ought to require this check at the state machine level - I think that many ETH wallets I've seen have warnings to prevent this "transaction-nightmare" but it nonetheless still happened - There is no reason this should be permitted to occur on anyone's custom tooling as well as the CLI

@fedekunze
Copy link
Collaborator

agree here with only covering standard transfers. I think both proposals are great 👍

@fedekunze
Copy link
Collaborator

is this something we want support ? @jbibla @faboweb @sabau what are your thoughts ?

@github-actions
Copy link
Contributor

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale label Jul 17, 2020
@tac0turtle tac0turtle removed the stale label Apr 3, 2023
@tac0turtle tac0turtle mentioned this issue Jan 3, 2024
10 tasks
@tac0turtle
Copy link
Member

closing this in favour of bankv2 epic. #17579

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

No branches or pull requests

5 participants