-
Notifications
You must be signed in to change notification settings - Fork 0
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
feat: add new amounts to a periodic vesting account #136
Conversation
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.
Looking really good! I'm convinced that this is close to the minimal (tested) change needed for this feature.
I would like to see a separate consume(...)
helper. Ping me again when you want another review.
x/auth/vesting/msg_server.go
Outdated
period := types.Period{ | ||
Length: nextP - time, | ||
Amount: p[iP].Amount, | ||
} | ||
merged = append(merged, period) | ||
timeP = nextP | ||
time = nextP | ||
iP++ |
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.
Could you please factor this logic out into a consume
function? It could be either a local-to-mergePeriods closure to avoid passing common arguments, or a file-level helper function.
That would help keep things DRY, make it easier for me to review, and avoid copy-pasta errors.
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.
I've refactored along the lines you suggested and I think it's a net improvement. It still uses a grab-bag of function-local variables to hold the iteration state, but I think data abstraction would be more trouble than it's worth.
Another approach I considered was to convert the vesting schedules to use absolute time, merge the schedules more simply, then convert back to relative times. Again, I don't think it's worth the effort.
PTAL. Look especially at |
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.
Looks great!
870b397
to
0eaeea9
Compare
Description
Refs: Agoric/agoric-sdk#3956
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!
to the type prefix if API or client breaking changeCHANGELOG.md
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!
in the type prefix if API or client breaking change