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

fix: traverseNodes unexpected behaviour #718

Merged
merged 12 commits into from
Apr 14, 2023

Conversation

catShaark
Copy link
Contributor

@catShaark catShaark commented Mar 23, 2023

  • If there's no update in the latest version of the tree, we set the root node's value to be the previous root node's key instead of its marshalled bytes. When we call traverseNodes, we iterate through every nodes (including root nodes) and unmarshal the marshalled node's value via MakeNode. MakeNode is made only for unmarshaling a marshalled node's value, so if a marshalled node's value is actually a node's key like described above, it'll handle incorrectly.
  • Suggest that in traverseNodes we check if the marshalled node's value is indeed a root node's key so that we can ignore it

@catShaark catShaark requested a review from a team as a code owner March 23, 2023 16:58
@catShaark catShaark changed the title traverseNodes unexpected behaviour fix: traverseNodes unexpected behaviour Mar 23, 2023
@cool-develope cool-develope self-assigned this Mar 27, 2023
@cool-develope
Copy link
Collaborator

@catShaark , thanks for catching it, it seems like we need to refactor the traverseNode function using prefixIterate
FYI, it is a test utility function (not critical part).

@catShaark
Copy link
Contributor Author

should I close this now

@cool-develope
Copy link
Collaborator

should I close this now

or you can modify it

@catShaark
Copy link
Contributor Author

ohh, I'll modify this

@catShaark
Copy link
Contributor Author

adding more cases to the test

@tac0turtle
Copy link
Member

@catShaark can we get a changelog here then we can merge

@catShaark
Copy link
Contributor Author

Yess

@catShaark
Copy link
Contributor Author

@tac0turtle, changelog added

@tac0turtle tac0turtle merged commit 32a89a0 into cosmos:master Apr 14, 2023
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