Skip to content
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

Merged
merged 29 commits into from
Feb 6, 2025
Merged

Conversation

natanasow
Copy link
Contributor

@natanasow natanasow commented Feb 4, 2025

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
  • The most straightforward way of getting protobufs response codes is just parsing .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.
  • Once we get the response codes, we start to build the contract body. It's just a string concatenation so nothing special.
  • As you may notice, there are no comments in the generated 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.
  • The next step is compiling the contract and making sure the newly created contract is not broken.
.github/workflows/update-hedera-response-codes.yml
  • Add a manually triggered GitHub Action to run the script above when we want and not on every release.
  • At the end of the action, we create a PR with the newly created contract. That's how would like a PR generated by the action.

Ad-hoc questions: #1219 (comment)

Related issue(s):

Fixes #1219

Notes for reviewer:

Checklist

  • Documented (Code comments, README, etc.)
  • Tested (unit, integration, etc.)

Signed-off-by: nikolay <n.atanasow94@gmail.com>
@natanasow natanasow self-assigned this Feb 4, 2025
@natanasow natanasow added the enhancement New feature or request label Feb 4, 2025
@natanasow natanasow added this to the 0.12.0 milestone Feb 4, 2025
Copy link

github-actions bot commented Feb 4, 2025

Test Results

 17 files  ± 0  101 suites  ±0   25m 23s ⏱️ + 2m 35s
368 tests +18  349 ✅ +22  19 💤 ±0  0 ❌  - 4 
411 runs  ± 0  390 ✅ + 3  21 💤 +1  0 ❌  - 4 

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.
"before all" hook for "should NOT be able to use transferFrom on fungible tokens without approval" ‑ TokenTransferContract Test Suite "before all" hook for "should NOT be able to use transferFrom on fungible tokens without approval"
"before all" hook for "should cancel a pending airdrop for a fungible token (FT)" ‑ HIP904 IHRC904Facade ContractTest Suite "before all" hook for "should cancel a pending airdrop for a fungible token (FT)"
"before each" hook for "Should check if an address is another address's operator" ‑ @OZERC1155Token Test Suite "before each" hook for "Should check if an address is another address's operator"
Should NOT allow a non-operator to transfer tokens to another account ‑ @OZERC1155Token Test Suite Should NOT allow a non-operator to transfer tokens to another account
Should NOT burn insufficient amount of token ‑ @OZERC1155Token Test Suite Should NOT burn insufficient amount of token
Should NOT transfer the ownership to another account if the caller is not owner ‑ @OZERC1155Token Test Suite Should NOT transfer the ownership to another account if the caller is not owner
Should allow an operator to transfer a token to another account ‑ @OZERC1155Token Test Suite Should allow an operator to transfer a token to another account
Should allow an operator to transfer tokens in batch to another account ‑ @OZERC1155Token Test Suite Should allow an operator to transfer tokens in batch to another account
Should burn token in batch ‑ @OZERC1155Token Test Suite Should burn token in batch
Should burn token ‑ @OZERC1155Token Test Suite Should burn token
Should check if an address is another address's operator ‑ @OZERC1155Token Test Suite Should check if an address is another address's operator
Should retrieve the token uri of a tokenID ‑ @OZERC1155Token Test Suite Should retrieve the token uri of a tokenID
Should set a new token URI ‑ @OZERC1155Token Test Suite Should set a new token URI
…

♻️ This comment has been updated with latest results.

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>
@natanasow natanasow marked this pull request as ready for review February 4, 2025 14:26
@natanasow natanasow requested review from a team, georgi-l95 and Ivo-Yankov as code owners February 4, 2025 14:26
Copy link

@Ferparishuertas Ferparishuertas left a 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

.github/workflows/update-hedera-response-codes.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@acuarica acuarica left a 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.

@acuarica
Copy link
Contributor

acuarica commented Feb 4, 2025

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?

Copy link
Member

@quiet-node quiet-node left a 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

.github/scripts/hedera-response-codes-protobuf-parser.js Outdated Show resolved Hide resolved
.github/workflows/update-hedera-response-codes.yml Outdated Show resolved Hide resolved
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>
Copy link
Contributor

@acuarica acuarica left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking good

contracts/system-contracts/HederaResponseCodes.sol Outdated Show resolved Hide resolved
quiet-node
quiet-node previously approved these changes Feb 5, 2025
Copy link
Member

@quiet-node quiet-node left a 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!

mishomihov00
mishomihov00 previously approved these changes Feb 6, 2025
Copy link
Contributor

@mishomihov00 mishomihov00 left a 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>
@natanasow natanasow dismissed stale reviews from mishomihov00 and quiet-node via 8d6e963 February 6, 2025 07:52
Copy link
Contributor

@mishomihov00 mishomihov00 left a 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

Copy link
Contributor

@acuarica acuarica left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great stuff!

Copy link
Member

@quiet-node quiet-node left a 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!

@natanasow natanasow merged commit 3aa1542 into main Feb 6, 2025
36 checks passed
@natanasow natanasow deleted the 1219-test-new-ci branch February 6, 2025 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
5 participants