-
Notifications
You must be signed in to change notification settings - Fork 55
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: automatic update of HederaResponseCodes.sol
#1220
Conversation
Signed-off-by: nikolay <n.atanasow94@gmail.com>
Test Results 17 files ± 0 101 suites ±0 25m 23s ⏱️ + 2m 35s Results for commit 8d6e963. ± Comparison against base commit 7f00646. This pull request removes 3 and adds 21 tests. Note that renamed tests count towards both.
♻️ This comment has been updated with latest results. |
181f9d4
to
8388f54
Compare
Signed-off-by: nikolay <n.atanasow94@gmail.com>
Signed-off-by: nikolay <n.atanasow94@gmail.com>
Signed-off-by: nikolay <n.atanasow94@gmail.com>
Signed-off-by: nikolay <n.atanasow94@gmail.com>
Signed-off-by: nikolay <n.atanasow94@gmail.com>
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
Added some comments
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.
Thanks, it's looking good.
I'm not convinced we should remove the comments. The code names in themselves are not that descriptive. Having comments at least provide a bit more context. This file is "exported" to users (they copy it to their own projects), that's why we should keep comments. Maybe using another JS proto library? |
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.
Great work on this! Awesome outcome and can save lots of time updating the contract! Left some suggestions and questions
Signed-off-by: nikolay <n.atanasow94@gmail.com>
Signed-off-by: nikolay <n.atanasow94@gmail.com>
Signed-off-by: nikolay <n.atanasow94@gmail.com>
Signed-off-by: nikolay <n.atanasow94@gmail.com>
Signed-off-by: nikolay <n.atanasow94@gmail.com>
Signed-off-by: nikolay <n.atanasow94@gmail.com>
Signed-off-by: nikolay <n.atanasow94@gmail.com>
Signed-off-by: nikolay <n.atanasow94@gmail.com>
Signed-off-by: nikolay <n.atanasow94@gmail.com>
Signed-off-by: nikolay <n.atanasow94@gmail.com>
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 good
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.
Great work! Approve to unblock!
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.
Review applies to .github/workflows/update-hedera-response-codes.yml
Signed-off-by: nikolay <n.atanasow94@gmail.com>
8d6e963
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.
Review applies to .github/workflows/update-hedera-response-codes.yml
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.
Great 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! Fantastic work as always!
Motivation:
Currently, there is no mechanism for keeping in sync protobufs response codes and these defined in
HederaResponseCodes.sol
contract.Implementation:
.github/scripts/hedera-response-codes-protobuf-parser.js
.proto
file and formatting it as we want. That's a bit overhead when we can get already parsed latest response codes by@hashgraph/proto
npm lib. So first step is to obtain all actual response codes.HederaResponseCodes.sol
contract because@hashgraph/proto
doesn't return any related comments. The "solution" of that is just adding a disclaimer at the top of the contract pointing out the source of response codes -response_code.proto
in the services repo..github/workflows/update-hedera-response-codes.yml
Ad-hoc questions: #1219 (comment)
Related issue(s):
Fixes #1219
Notes for reviewer:
Checklist