-
Notifications
You must be signed in to change notification settings - Fork 46
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
FIX: Inspecting reward accounts uses wrong AddressInfo
field for staking scripts
#268
base: master
Are you sure you want to change the base?
FIX: Inspecting reward accounts uses wrong AddressInfo
field for staking scripts
#268
Conversation
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 @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.
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
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 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. |
Actually I did a grep of the code base and |
@fallen-icarus how is it cli tests are falling?
?
All passing now? |
Co-authored-by: Pawel Jakubas <jakubas.pawel@gmail.com>
@paweljakubas I'm not using nix. I was originally doing It turned out the extra But my original approach to running the tests is still showing 92 of 344 tests failing. I don't understand why |
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 |
I would expect that inspecting a reward account address would use the
infoStakeScriptHash
field ofAddressInfo
, but it is usinginfoScriptHash
. The pubkey version is usinginfoStakeKeyHash
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.