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

imp (claims): validate authorized channels in message to update claims params #1378

Merged
merged 16 commits into from
Feb 13, 2023

Conversation

MalteHerrmann
Copy link
Contributor

Description

This PR adds the validation of the authorized channels in a MsgUpdateParams before applying the changes.
It iterates through the available channels in app.ibcKeeper.ChannelKeeper and checks if the channels are present there.


Closes ENG-1400

@linear
Copy link

linear bot commented Feb 10, 2023

ENG-1400 Stateful Validation of claims params against stored values

Claim params values are not currently verified when using MsgUpdateParams . This could potentially cause that an invalid (non existent or closed) channel is registered in the module params

Scope of Work

  • Check that claims Params.AuthorizedChannels exist in the channel keeper store (i.e call GetChannel and check that found is true)
  • Check that claims Params.EvmChannels exist in the channel keeper store (i.e call GetChannel and check that found is true)
  • Check that the channel state status is Open

@codecov
Copy link

codecov bot commented Feb 10, 2023

Codecov Report

Merging #1378 (b8cf4f3) into main (7e98d8f) will increase coverage by 0.04%.
The diff coverage is 100.00%.

❗ Current head b8cf4f3 differs from pull request most recent head 4f22388. Consider uploading reports for the commit 4f22388 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1378      +/-   ##
==========================================
+ Coverage   72.23%   72.27%   +0.04%     
==========================================
  Files         259      259              
  Lines       17614    17640      +26     
==========================================
+ Hits        12724    12750      +26     
  Misses       4321     4321              
  Partials      569      569              
Impacted Files Coverage Δ
app/app.go 82.09% <100.00%> (ø)
x/claims/keeper/keeper.go 85.18% <100.00%> (+0.56%) ⬆️
x/claims/keeper/msg_server.go 91.17% <100.00%> (+24.50%) ⬆️

@MalteHerrmann MalteHerrmann marked this pull request as ready for review February 10, 2023 10:45
@MalteHerrmann MalteHerrmann requested review from 0a1c and removed request for a team February 10, 2023 10:45
Copy link
Contributor

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

Thanks @MalteHerrmann! left some minor comments

x/claims/keeper/msg_server.go Show resolved Hide resolved
x/claims/keeper/msg_server.go Outdated Show resolved Hide resolved
x/claims/keeper/msg_server.go Outdated Show resolved Hide resolved
x/claims/keeper/msg_server_test.go Outdated Show resolved Hide resolved
x/claims/keeper/msg_server.go Outdated Show resolved Hide resolved
Copy link
Contributor

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

ACK, pending bug fix entry

x/claims/keeper/msg_server.go Outdated Show resolved Hide resolved
x/claims/keeper/msg_server.go Outdated Show resolved Hide resolved
@fedekunze
Copy link
Contributor

@MalteHerrmann can you look into the tests and linter errors?

@facs95 facs95 enabled auto-merge (squash) February 13, 2023 21:56
@facs95 facs95 merged commit f54544a into main Feb 13, 2023
@facs95 facs95 deleted the malte/validate-claims-params branch February 13, 2023 22:06
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.

None yet

3 participants