hashgraph / hedera-sdk-js

Hedera™ Hashgraph SDK for JavaScript/TypeScript
https://docs.hedera.com/guides/docs/sdks
Apache License 2.0
276 stars 144 forks source link

fix: incorrect protobuf body field used #2613

Closed kantorcodes closed 4 weeks ago

kantorcodes commented 4 weeks ago

Description:

This PR fixes _fromProtobuf inside of TokenUpdateNftsTransaction to use the correct tokenUpdateNfts field instead of tokenUpdate. Upon investigating https://github.com/hashgraph/hedera-wallet-connect/issues/265, we realized this property was not being referenced properly.

Related issue(s):

Fixes https://github.com/hashgraph/hedera-wallet-connect/issues/265

Notes for reviewer:

You can confirm the error happens outside of this branch by running the following code:

import Long from "long";
import { TokenUpdateNftsTransaction, Transaction } from "./exports.js";

const updateNftsTransaction = new TokenUpdateNftsTransaction()
    .setTokenId("0.0.123")
    .setSerialNumbers([Long.fromInt(1)])
    .setMetadata(new TextEncoder().encode("metadata"));

const parsedTx = Transaction.fromBytes(updateNftsTransaction.toBytes());

console.log("parsedTx: ", parsedTx._makeTransactionBody());

Checklist

codecov[bot] commented 4 weeks ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 84.77%. Comparing base (cf51a5a) to head (bda8dfe). Report is 1 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #2613 +/- ## ========================================== + Coverage 84.72% 84.77% +0.04% ========================================== Files 283 283 Lines 71094 71094 ========================================== + Hits 60234 60269 +35 + Misses 10860 10825 -35 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

ivaylogarnev-limechain commented 4 weeks ago

Hi @kantorcodes , thank you for submitting this PR! It looks great.

Would you mind adding a unit test for this change in the TokenUpdateTransaction.js to satisfy the code coverage in the CI check?

0xivanov commented 4 weeks ago

Nice catch! This should be the correct behaviour.

Looks like we were missing some unit tests for this class as a whole and we also have a TokenNftsUpdateTransaction class which should be deleted. @ivaylogarnev-limechain do you think we can do this in a follow up issue?

ivaylogarnev-limechain commented 4 weeks ago

Nice catch! This should be the correct behaviour.

Looks like we were missing some unit tests for this class as a whole and we also have a TokenNftsUpdateTransaction class which should be deleted. @ivaylogarnev-limechain do you think we can do this in a follow up issue?

For sure, that sounds like a better option.

agadzhalov commented 4 weeks ago

Hi @kantorcodes thanks for your effort. Your commit is not verified, it's signed, but it's not verified which may cause us problems in the future, can you try to sign it again may be using ammend. I can also see that your commits are not verified in other repos.

Are you sure you've setup your GPG keys properly? You can look at this page for more info (Signing commits).

image
kantorcodes commented 4 weeks ago

Hi @kantorcodes thanks for your effort. Your commit is not verified, it's signed, but it's not verified which may cause us problems in the future, can you try to sign it again may be using ammend. I can also see that your commits are not verified in other repos.

Are you sure you've setup your GPG keys properly? You can look at this page for more info (Signing commits).

image

Thanks! I think I've got these signed properly now :)

sonarcloud[bot] commented 4 weeks ago

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

kantorcodes commented 4 weeks ago

commits must be verified

Thanks for taking a look. I think commits should be signed properly. I realized afterwards (and likely why this went unnoticed initially) that fromProtobuf had no test coverage and this seems to have blocked the merge, so I added another commit to add a basic test in as well.