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

Turn on rest binary calculation tests #3360

Merged
merged 5 commits into from
Jun 28, 2022

Conversation

paweljakubas
Copy link
Contributor

@paweljakubas paweljakubas commented Jun 27, 2022

Comments

Issue Number

adp-1885

Comment on lines 1670 to 1672
case era of
ShelleyBasedEraAlonzo ->
slimCBOR eraSerializedCBOR
Copy link
Member

Choose a reason for hiding this comment

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

Is it not saner to also add Alonzo-specific expected values in your case expressions above, like you already have for Babbage?

Copy link
Member

Choose a reason for hiding this comment

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

In fact, the Alonzo and Babbage binaries are the same, no? I haven't run and tested, but it looks like IntersectMBO/cardano-ledger#2863 should have affected Alonzo as well.

Then having the same expected value for Alonzo and Babbage sounds much easier, and you could still keep your comment explaining the difference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks Johannes for your look. I tend to agree and now we have binary rep for Babbage, Alonzo and the rest. There are differences between all of them. I added the comment illustrating the point in the relevant test:

1421 -- Up till Mary era we have the following structure of transaction                                                                                                                      
1422 --   transaction =                                                                                                                                                                      
1423 -- [ transaction_body                                                                                                                                                                   
1424 -- , transaction_witness_set                                                                                                                                                            
1425 -- , auxiliary_data / null                                                                                                                                                              
1426 -- ]                                                                                                                                                                                    
1427 -- So we begin with 3-element array binary prefix, that is encoded as '83'                                                                                                              
1428 -- From Alonzo on tx was enriched for isValid field making it                                                                                                                           
1429 -- 4-element array that is encoded as '84'.                                                                                                                                             
1430 --   transaction =                                                                                                                                                                      
1431 -- [ transaction_body                                                                                                                                                                   
1432 -- , transaction_witness_set                                                                                                                                                            
1433 -- , bool                                                                                                                                                                               
1434 -- , auxiliary_data / null                                                                                                                                                              
1435 -- ]                                                                                                                                                                                    
1436 -- Alonzo transaction stil has the same binary representation (array)                                                                                                                   
1437 -- for transaction outputs like in the previous eras.                                                                                                                                   
1438 -- Babbage era changes this representation, and introduces map binary                                                                                                                   
1439 -- representation for transaction outputs as the contept of them is                                                                                                                     
1440 -- extended in this era.                                    

Copy link
Member

Choose a reason for hiding this comment

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

Ah ok so they are different after all 👌

@paweljakubas paweljakubas force-pushed the paweljakubas/turn-on-rest-binary-calculation-tests branch from 2306c98 to 08fda96 Compare June 28, 2022 06:55
-- Alonzo transaction stil has the same binary representation (array)
-- for transaction outputs like in the previous eras.
-- Babbage era changes this representation, and introduces map binary
-- representation for transaction outputs as the contept of them is
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
-- representation for transaction outputs as the contept of them is
-- representation for transaction outputs as the concept of them is

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

Comment on lines 1670 to 1672
case era of
ShelleyBasedEraAlonzo ->
slimCBOR eraSerializedCBOR
Copy link
Member

Choose a reason for hiding this comment

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

Ah ok so they are different after all 👌

@paweljakubas
Copy link
Contributor Author

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jun 28, 2022

Build succeeded:

@iohk-bors iohk-bors bot merged commit 67e519d into master Jun 28, 2022
@iohk-bors iohk-bors bot deleted the paweljakubas/turn-on-rest-binary-calculation-tests branch June 28, 2022 11:49
WilliamKingNoel-Bot pushed a commit that referenced this pull request Jun 28, 2022
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.

2 participants