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: display tx hash instead of txi when tx_hash=true #789

Merged
merged 3 commits into from
Jul 19, 2022

Conversation

sborrazas
Copy link
Contributor

@sborrazas sborrazas commented Jul 15, 2022

refs #764

@sborrazas sborrazas requested review from thepiwo and jyeshe July 15, 2022 17:53
@sborrazas sborrazas self-assigned this Jul 15, 2022
Comment on lines +414 to +418
Keyword.get(opts, :expand?, false) ->
Txs.fetch!(state, txi)

Keyword.get(opts, :tx_hash?, false) ->
Enc.encode(:tx_hash, Txs.txi_to_hash(state, txi))
Copy link
Member

Choose a reason for hiding this comment

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

It would be good for the user to get some kind of exception like this otherwise the param gets ignored silently. This would indicate that it should be used one param or the other.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, added this validation on the PaginationPlug so we didn't have to on every top module

else
txi
cond do
Keyword.get(opts, :expand?, false) ->
Copy link
Member

Choose a reason for hiding this comment

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

same as on names.ex

Comment on lines +867 to +868
{:ok, _encoded_tx_hash},
:aeser_api_encoder.safe_decode(:tx_hash, tx_hash)
Copy link
Member

Choose a reason for hiding this comment

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

It could be checked that besides present the tx_hash is correct, here on integration or on unit tests.

An integration test for oracle would be missing or if the tx_hash becomes validated on oracle unit test it would be enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, added oracle test which validates this now

Copy link
Member

@jyeshe jyeshe left a comment

Choose a reason for hiding this comment

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

Could it test if the hash is correct for each oracle (having it's own tx_hash)?

@sborrazas
Copy link
Contributor Author

Could it test if the hash is correct for each oracle (having it's own tx_hash)?

I think it's enough to test that at least one tx hash was converted, testing that the conversion txi->hash is not always the same seems to be too far fetch for values that were chosen randomly (8989-> th_5aUc7Xqk3TkG38XVjCzZPAWc1geGFfjGcgEytcZAFe3FJyn4g)

Copy link
Member

@jyeshe jyeshe left a comment

Choose a reason for hiding this comment

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

For a complete test, it would be to test if the hash belongs to the correct oracle.

@sborrazas sborrazas merged commit 295da57 into master Jul 19, 2022
@sborrazas sborrazas deleted the tx_hash-option branch July 19, 2022 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants