FIX: Inspecting reward accounts uses wrong AddressInfo field for staking scripts

[fallen-icarus]

I would expect that inspecting a reward account address would use the infoStakeScriptHash field of AddressInfo, but it is using infoScriptHash. The pubkey version is using infoStakeKeyHash so this seems like a bug. I ran the tests both before and after the change, and all tests pass; they don't seem to cover this.

If this is the intended behavior, feel free to close this PR.

[paweljakubas]

thanks @fallen-icarus . good catch! I see for historical reasons we have infoScriptHash for spending script hash credential. It should be called rather infoSpendingScriptHash - I will do this renaming in the separate PR in case you are not eager to do it in the current one:-) Please also add in Changelog.md entry "Fix field for staking scripts in AddressInfo" - the log for the incoming release. We will take care of merging this PR soon.

[Crypto2099]

LGTM

[fallen-icarus]

I wasn't sure if the changelog entry should be in a "Fixed" category or not so I just put it under "Added". I figured it could be reformatted latter, if necessary.

I see for historical reasons we have infoScriptHash for spending script hash credential. It should be called rather infoSpendingScriptHash - I will do this renaming in the separate PR in case you are not eager to do it in the current one:-)

I would rather you do the renaming since I am not as familiar with the code base 😅. I'm not sure if tests or documentation would also need to be updated.

[fallen-icarus]

Actually I did a grep of the code base and infoScriptHash only seems to be used in that module. So I did a search & replace and re-ran the tests. The core tests are passing, but the command-line tests are failing. The command-line tests were failing even before I made changes, though.

[paweljakubas]

@fallen-icarus how is it cli tests are falling?
You are going to nix develop and then in develop shell you run

cabal test cardano-addresses-cli:unit

?
If yes, what about after going to development shell

$ export LANG=C.UTF-8
$ cabal test cardano-addresses-cli:unit
$ cabal test cardano-addresses:unit

All passing now?

[fallen-icarus]

@paweljakubas I'm not using nix. I was originally doing cabal run tests from withing the command-line directory, and this results in 92 of 344 tests failing. I just tried your command cabal test cardano-addresses-cli:unit, and this results in only 2 of 344 tests failing.

It turned out the extra infoScriptHash -> infoStakeScriptHash you found in the json is what was causing the 2 tests to fail in the latter run. Now, running cabal test cardano-addresses-cli:unit is showing all tests pass.

But my original approach to running the tests is still showing 92 of 344 tests failing. I don't understand why cabal run tests and cabal test cardano-addresses-cli:unit would produce different results. I would think they would be equivalent.

[fallen-icarus]

I was looking over the automated checks to see why they were failing and saw this typescript test was failing. The code is shown below:

    it('Shelley Stake Shared network tag 0', () => expect(inspectAddress("stake17pshvetj09hxjcm9v9jxgunjv4ehxmr0d3hkcmmvdakx7mq36s8xc")).resolves.toEqual({
      "address_style": "Shelley",
      "address_type": 15,
      "network_tag": 0,
      "spending_shared_hash": "61766572796e69636561646472726573736c6f6c6f6c6f6c6f6c6f6c",
      "spending_shared_hash_bech32": "addr_shared_vkh1v9mx2unede5kxetpv3j8yun9wdekcmmvdakx7mr0d3hkcuuhu9r",
      "stake_reference": "by value",
      "stake_shared_hash": "61766572796e69636561646472726573736c6f6c6f6c6f6c6f6c6f6c",
      "stake_shared_hash_bech32": "stake_shared_vkh1v9mx2unede5kxetpv3j8yun9wdekcmmvdakx7mr0d3hkcjta3en",
    }));

I suspect the issue is that the spending_shared_hash fields should not actually be present since it is a staking address, but I am not familiar enough with the address types to know for sure. I think the jsonHash changes in this PR is what is now causing the test to fail since the infoScriptHash field was improperly being used for both spending_shared_hash and stake_shared_hash. I have zero experience with typescript so I don't feel comfortable touching this test, but I was hoping this PR could get merged soon.