-
Notifications
You must be signed in to change notification settings - Fork 48
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
Min Initial Deposit (Recreated using main as base branch) #138
Conversation
@nghuyenthevinh2000: I recreated this PR (now based non main). Can you test the upgrade handler/migration for this? I fear the automated upgrade test is really doing nothing... I would appreciate you manual test that you have been doing for #74. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM m8. Only have a few minor cosmetic suggestions :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Base logic looks fine
- a test should be needed
- func (AppModule) ConsensusVersion() uint64 { return 2 } in x/treasury/module.go should return 3
- you need to add an upgrade handler in app/upgrades as well since this is state migration in x/treasury. It is also prerequisite to run upgrade-test ci
bf846fd
to
8172168
Compare
i wrecked this branch argh - that's because I do things in a hurry........ |
Ok. I saved the branch 🤣. Wrote another test for this (half-automated spinning single validator node doing the min initial deposit prop via CLI). Where do I put this script? @nghuyenthevinh2000 : I also comply with your change requests. Can you resolve them? |
Normally ad-hoc scripts would go into the "contrib" folder, but I recon we can drop it in the new "tests" folder we are setting up for the automated testing stuff :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Summary of changes
See #74 and #41.
Report of required housekeeping
(FOR ADMIN) Before merging