refactor: improve docs, comments, tests, and small refactors #303
Conversation
JimLarson
added
documentation
| - `--months`: The number of months to vest over. | ||
| - `--time`: The time of day of the vesting event, in 24-hour HH:MM format. | ||
| Defaults to midnight. | ||
| - `"start_time"`: integer UNIX time; |
There was a problem hiding this comment.
| - `"start_time"`: integer UNIX time; | |
| - `"start_time"`: integer seconds since the POSIX epoch; |
There was a problem hiding this comment.
I'll stick with "UNIX time" for the concept, since a) it matches the the Go time package API (which doesn't mention an epoch, only "UNIX time"), and b) "UNX time" has its own wikipedia page - "POSIX time" redirects there.
| stdout. The following flags control the output: | ||
| When the `--write` flag is set, the tool will write a schedule to stdout. | ||
| The format is the the format expected by the `create-periodic-vesting-account` | ||
| or `create-clawback-vesting-account` cli commands, namely a JSON |
There was a problem hiding this comment.
| or `create-clawback-vesting-account` cli commands, namely a JSON | |
| or `create-clawback-vesting-account` cli commands, namely JSON text representing an |
There was a problem hiding this comment.
JSON implies text. However, from all the format-related comments, it could be confusing to have it spread out. I've added a single format section with an example that I hope makes it more clear.
| - `--time`: The time of day of the vesting event, in 24-hour HH:MM format. | ||
| Defaults to midnight. | ||
| - `"start_time"`: integer UNIX time; | ||
| - `"periods"`: a JSON array of JSON objects with members: |
There was a problem hiding this comment.
I'm assuming there's not another layer of serialization here (e.g., output looks like { …, "periods": [ { "coins": "…", "length_seconds": <digits> }, … ] }).
| - `"periods"`: a JSON array of JSON objects with members: | |
| - `"periods"`: an array of objects with members: |
There was a problem hiding this comment.
Correct - I've added an example and moved this up to a "format" section, where the JSON context is more clear and I can drop the superfluous and confusing repeats.
| Defaults to midnight. | ||
| - `"start_time"`: integer UNIX time; | ||
| - `"periods"`: a JSON array of JSON objects with members: | ||
| - "coins": string giving the text coins format for the amount vested at this event; |
There was a problem hiding this comment.
I think example output would help, especially for this field. Probably just a URL pointing to something that already exists.
There was a problem hiding this comment.
I don't see a URL for the coins text format. I'll add a multi-denom example.
| The following flags control the output: | ||
|
|
||
| - `--coins:` The total coins to vest, e.g. `100ubld,50urun`. | ||
| - `--months`: The number of monthly to vesting events. |
There was a problem hiding this comment.
I think there's a typo here, but the explanation could still be a little more clear.
| - `--months`: The number of monthly to vesting events. | |
| - `--months`: The total number of months to complete vesting. |
| - `--start`: The vesting start time: i.e. the first event happens in the | ||
| next month. Specified in the format `YYYY-MM-DD` or `YYYY-MM-DDThh:mm`, | ||
| e.g. `2006-01-02T15:04` for 3:04pm on January 2, 2006. | ||
| - `--cliffs`: One or more vesting cliffs in `YYYY-MM-DD` or `YYYY-MM-DDThh:mm` | ||
| - `--time`: The time of day of the vesting events, in 24-hour HH:MM format. |
There was a problem hiding this comment.
--start and --time are interpreted as UTC, right? And does --time override hours and minutes in --start?
There was a problem hiding this comment.
No. There's a note above that times are interpreted in the local timezone. I added a repeat of that comment here to clarify. The time-of-day of the start time has no bearing on the time-of-day of the later vesting events.
| day of the month (or the last day of the month, if the month does not have a | ||
| sufficient number of days), for the specified number of months. The total coins | ||
| to vest will be divided as evenly as possible among all the vesting events. | ||
| Lastly, all events before the last cliff time, if any, are consolidated into a single even |
There was a problem hiding this comment.
| Lastly, all events before the last cliff time, if any, are consolidated into a single even | |
| Lastly, all events before the last cliff time, if any, are skipped and consolidated into a single event |
There was a problem hiding this comment.
I think "consolidated" is sufficient.
| @@ -81,6 +98,7 @@ func divideCoins(coins sdk.Coins, divisor int) ([]sdk.Coins, error) { | |||
| // monthlyVestTimes generates timestamps for successive months after startTime. | |||
| // The monthly events occur at the given time of day. If the month is not | |||
| // long enough for the desired date, the last day of the month is used. | |||
| // The number of months must be postive. | |||
| // readCmd reads periods in JSON from stdin and writes a sequence of vesting | ||
| // events in local time to stdout. | ||
| // readCmd reads a schedule file from stdin and writes a sequence of vesting | ||
| // events in local time to stdout. See README.md for the format. |
There was a problem hiding this comment.
In local time? Oof. If that's the case we should probably find a way to convey UTC offsets.
There was a problem hiding this comment.
I think it matches the anticipated usage pattern. The comment about timezones in README.md describes how to get other timezones.
| @@ -13,6 +13,9 @@ import ( | |||
| "github.com/cosmos/cosmos-sdk/x/auth/vesting/client/cli" | |||
| ) | |||
|
|
|||
| // vestcalc is a utility for creating or reading period files | |||
There was a problem hiding this comment.
Vocabulary consistency nit.
| // vestcalc is a utility for creating or reading period files | |
| // vestcalc is a utility for creating or reading schedule files |
|
I've addressed your feedback, @gibson042 , PTAL. I went along with most suggestions, either by letter or spirit. If you feel strongly about any changes I resisted, please re-raise them. |
Description
Improve the documentation and comments to enable local auditing of all code.
Incidental small refactoring and test expansion.
Test coverage could be improved, but not in this PR.
Author Checklist
I have...
!to the type prefix if API or client breaking changeCHANGELOG.mdReviewers 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