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.

Sign up for GitHub

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

Jump to bottom

fix: reject modifying NFT class with token index filled in MsgModify #1102

Merged
merged 5 commits into from
Aug 30, 2023
Merged

fix: reject modifying NFT class with token index filled in MsgModify #1102

merged 5 commits into from
Aug 30, 2023

Conversation

0Tech
Copy link
Collaborator

@0Tech 0Tech commented Aug 29, 2023 •
edited
Loading

Description

closes: #1101

Checklist:

  • I followed the contributing guidelines and code of conduct.
  • I have added a relevant changelog to CHANGELOG.md
  • I have added tests to cover my changes.
  • I have updated the documentation accordingly.
  • I have updated API documentation client/docs/swagger-ui/swagger.yaml

@0Tech 0Tech self-assigned this Aug 29, 2023
@0Tech 0Tech added A: bug Something isn't working C:x/collection labels Aug 29, 2023
@0Tech 0Tech added this to the v0.48.0 milestone Aug 29, 2023
@codecov
Copy link

codecov bot commented Aug 29, 2023 •
edited
Loading

Codecov Report

Merging #1102 (cc480bf) into rc/v0.48.0-rcx (3eddfec) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@               Coverage Diff               @@
##           rc/v0.48.0-rcx    #1102   +/-   ##
===============================================
  Coverage           69.69%   69.70%           
===============================================
  Files                 642      642           
  Lines               67277    67280    +3     
===============================================
+ Hits                46892    46895    +3     
  Misses              18203    18203           
  Partials             2182     2182
Files Changed Coverage Δ
x/collection/msgs.go 96.82% <100.00%> (+0.01%) ⬆️

@0Tech 0Tech marked this pull request as ready for review August 29, 2023 05:28
jaeseung-bae
jaeseung-bae previously approved these changes Aug 29, 2023
View reviewed changes
zemyblue
zemyblue reviewed Aug 29, 2023
View reviewed changes
Copy link
Member

@zemyblue zemyblue left a comment

Choose a reason for hiding this comment

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

What could be the cause?

@0Tech
Copy link
Collaborator Author

0Tech commented Aug 29, 2023

What could be the cause?

It violates the old spec. So I added logic to reject this edge case.

@0Tech 0Tech added the A: State Machine Breaking Any changes that result in a different AppState given same genesisState and txList. label Aug 29, 2023
zemyblue
zemyblue reviewed Aug 30, 2023
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@0Tech 0Tech merged commit c95052c into Finschia:rc/v0.48.0-rcx Aug 30, 2023
36 checks passed
@0Tech 0Tech deleted the fix-collecion-msgmodify branch August 30, 2023 07:03
@zemyblue zemyblue linked an issue Aug 30, 2023 that may be closed by this pull request
Try MsgModify with invalid tokenIndex, does not blocking. #1101
Closed
4 tasks
@zemyblue zemyblue mentioned this pull request Aug 31, 2023
Merged
4 tasks
@zemyblue zemyblue mentioned this pull request Oct 17, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaeseung-bae jaeseung-bae jaeseung-bae approved these changes

@zemyblue zemyblue zemyblue approved these changes

Assignees

@0Tech 0Tech

Labels
A: bug Something isn't working A: State Machine Breaking Any changes that result in a different AppState given same genesisState and txList. C:x/collection
Projects
None yet
Milestone
v0.48.0
Development

Successfully merging this pull request may close these issues.

Try MsgModify with invalid tokenIndex, does not blocking.
3 participants
@0Tech @zemyblue @jaeseung-bae