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

Move allow list logic to msg server #550

Merged
merged 5 commits into from
Nov 4, 2024
Merged

Conversation

dssei
Copy link
Contributor

@dssei dssei commented Nov 1, 2024

Describe your changes and provide context

Move allow list logic to msg server

Testing performed to validate your change

  • Unit tests
  • Local chain functional tests

@dssei dssei self-assigned this Nov 1, 2024
Copy link

codecov bot commented Nov 1, 2024

Codecov Report

Attention: Patch coverage is 45.45455% with 12 lines in your changes missing coverage. Please review.

Project coverage is 54.87%. Comparing base (71ec704) to head (4d5e05d).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
x/bank/keeper/msg_server.go 0.00% 12 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #550      +/-   ##
==========================================
- Coverage   54.89%   54.87%   -0.03%     
==========================================
  Files         631      631              
  Lines       54954    54926      -28     
==========================================
- Hits        30169    30138      -31     
- Misses      22628    22633       +5     
+ Partials     2157     2155       -2     
Files with missing lines Coverage Δ
x/bank/keeper/send.go 87.23% <100.00%> (+1.20%) ⬆️
x/bank/keeper/msg_server.go 2.70% <0.00%> (-0.33%) ⬇️

... and 2 files with indirect coverage changes

@@ -533,6 +534,19 @@ func (k BaseSendKeeper) IsAllowedToSendCoins(ctx sdk.Context, addr sdk.AccAddres
return true
}

func (k BaseSendKeeper) getAllModuleAddresses(ctx sdk.Context) []string {
Copy link
Contributor Author

@dssei dssei Nov 2, 2024

Choose a reason for hiding this comment

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

Not the most efficient way to retrieve all module addresses and we may also want to cache it. So, I will look over the weekend how to improve this. I have also found we are passing some module addresses via blockedAddrs map[string]bool, that is being used for blocking some transactions though. And there is also module.Manager that may return them. @philipsu522 @codchen let me know what would the most efficient way.

Copy link
Contributor

Choose a reason for hiding this comment

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

yea i agree this might be too expensive. What about iterating through all modules (maybe app.go could expose this because we register all modules) and then calling

func (ak AccountKeeper) GetModuleAddress(moduleName string) sdk.AccAddress {
on it? Not sure if @codchen has a better idea here though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactored, to avoid module checks altogether.
I think for this feature we should only be in business of controlling transfers for regular account/addresses, not modules, with transfers to modules implicitly not allowed via send. And we should not interfere with module->module, account->module and module->account flows.
The way to do this would be to move the allowlist logic to the message server and only do the checks in send and multisend.

@dssei dssei changed the title Add all modules to allowlist exceptions Move allow list logic to msg server Nov 4, 2024
@dssei dssei merged commit 4214e66 into main Nov 4, 2024
14 of 15 checks passed
@dssei dssei deleted the al_add_exceptions_for_all_modules branch November 4, 2024 19:27
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.

3 participants