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

add utils.SafeSendCoins() #1373

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Conversation

assafmo
Copy link
Contributor

@assafmo assafmo commented Mar 4, 2025

didn't change tests and past upgrade handlers

didn't change tests and past upgrade handlers
@assafmo assafmo changed the title add utils.SafeSendCoins() and update various modules to use it add utils.SafeSendCoins() Mar 4, 2025
Copy link
Collaborator

@sampocs sampocs left a comment

Choose a reason for hiding this comment

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

looks good!

I'm wondering if it would make more sense to just parameterize safe send coins with a checkBlockedAddr bool so that we could use it for every SendCoins instance instead of having to add the comment that says why we didn't use it

@assafmo
Copy link
Contributor Author

assafmo commented Mar 4, 2025

Good point. I'm not sure if it'll be more readable tho, from the outside it'll just be true/false. I'll do it and see how it looks.

Also, wdyt about adding a ci job to check that we're not using SendCoins anywhere in the future?

@sampocs
Copy link
Collaborator

sampocs commented Mar 4, 2025

Ooo CI job is a great idea! (assuming we stick with the switch to using SafeSend everywhere)

@assafmo
Copy link
Contributor Author

assafmo commented Mar 4, 2025

Yeah with the switch.

@assafmo assafmo force-pushed the add-safe-send-coins-util-func branch from e6312fd to 5af8c8d Compare March 5, 2025 06:57
@assafmo assafmo requested a review from sampocs March 5, 2025 14:59
Copy link
Collaborator

@sampocs sampocs left a comment

Choose a reason for hiding this comment

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

Looks good! one last request - I would revert the test files and change the CI job to ignore test files. I think it's fine to use the bank one for those since we want them to be as non-verbose as possible

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

Successfully merging this pull request may close these issues.

2 participants