-
Notifications
You must be signed in to change notification settings - Fork 4
Encode command for Protego CLI #15
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
Conversation
amusingaxl
left a comment
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 left some comments with possible improvements.
Co-authored-by: amusingaxl <112016538+amusingaxl@users.noreply.github.com>
…into feat/cli-encode
cli/README.md
Outdated
|
|
||
| ```bash | ||
| npm run cli -- --status PENDING --from-block 19420069 | ||
| npm run cli -- list --status PENDING --from-block 19420069 |
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 think this is outdated, as there's no cli script defined on package.json. Same thing goes for all below.
Additionally, encode is not present on the package.json as well, so there's no way to run it thru npm.
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.
Done in 5ac801e (just removed npm scripts all together).
| ◯ ↓ hash: 0xe163693acf1542472a882e888d91159f799383904ea4f3de4b58115f3740d5f2 | usr: 0x0c5fb8D0addBd19258BDbD9221D3F87D294Be590 | eta: 1749170292 | ||
| ``` | ||
|
|
||
| **Output:** |
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 think we should document in here the possible output formats, as it's currently just on the help message.
I also think it would be helpful to indicate which one should be used for Etherscan, for example.
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.
Don't we need to maybe include guy here as well?
This is the spell address, which is what users have contact with.
Ofc it's more relevant to compliant spells, but in a governance attack situation it might be useless.
Another thing: should we parse the timestamp and show a human-friendly date as well?
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.
Readme updated in 515e393.
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.
Regarding the date, I think it's a great idea. Was thinking of adding it just to the table view, something like:
╔═══════════════════════╤═══════════════════════════════════╤═══════════════════════╤═══════════════════════════════════╤════════════╤══════════════╤════════════╗
║ GUY │ HASH │ USR │ TAG │ FAX │ ETA │ STATUS ║
╟───────────────────────┼───────────────────────────────────┼───────────────────────┼───────────────────────────────────┼────────────┼──────────────┼────────────╢
║ 0x11378105b356039fC1C │ 0xe1ee25b3818453fe9f283b7f11bf9ea │ 0x1AF95B825DCb36cf0fB │ 0xb00835271ba99b9695e8413dbc40cf5 │ 0xc0406226 │ 1749170292 │ PENDING ║
║ 264019EF182EbE581e390 │ 6c21c062329c4fb86b7472e228fdece22 │ B4Ff3cD05cf752B6BFD55 │ 784d5bd971d38f0a085e60407eb61accb │ │ (6/5/2025) │ ║
╟───────────────────────┼───────────────────────────────────┼───────────────────────┼───────────────────────────────────┼────────────┼──────────────┼────────────╢
║ 0x11378105b356039fC1C │ 0x87721f1b7f036bce3eea2569dad5e3f │ 0x49cAA015f300949336f │ 0xb00835271ba99b9695e8413dbc40cf5 │ 0xc0406226 │ 1749170292 │ PENDING ║
║ 264019EF182EbE581e390 │ f2932413ff2ae26a8dcece0fec55ba4ed │ b3519e59C6a9b8401E1fB │ 784d5bd971d38f0a085e60407eb61accb │ │ (6/5/2025) │ ║
╟───────────────────────┼───────────────────────────────────┼───────────────────────┼───────────────────────────────────┼────────────┼──────────────┼────────────╢
What do you guys think? Should we also add it to the JSON output?
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.
No, not the JSON, as this is more targeted to be consumed by scripts.
For the table, it looks okay, I'd only change the format to yyyy-mm-dd.
Or maybe display the full date-time and the timestamp between parenthesis.
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 avoided the full date + timestamp because of the layout there, it'd ruin it (we'd need a larger column for ETA, and it's already crowded). Just the date fits well under the ETA IMO.
The current date there is formatted to the user locale (also adjusted to the user local time), should we stick with fixed formatting and UTC times?
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.
Yes, let's use UTC timezone for everything.
I just wonder how useful would it be to just display the date and not the time.
If we're thinking about a governance attack with multiple spells being scheduled in short time spans, there will be many of them with the same date, but probably different times.
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.
Ser, I think if we include the UTC suffix in the header, we can have enough room to display the seconds as well.
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.
Done in c1c881b.
amusingaxl
left a comment
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.
Please don't forget to update the docs as well.
amusingaxl
left a comment
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.
My 2 last nits ser.
Other than that, it's good to go.
GG.
amusingaxl
left a comment
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.
My final suggestions.
Other than that, it looks good (:
Co-authored-by: amusingaxl <112016538+amusingaxl@users.noreply.github.com>
amusingaxl
left a comment
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
Co-authored-by: amusingaxl <112016538+amusingaxl@users.noreply.github.com>
Co-authored-by: amusingaxl <112016538+amusingaxl@users.noreply.github.com>
amusingaxl
left a comment
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 ![]()
No description provided.