Pr feedback #200
Pr feedback #200
Conversation
| use crate::{Address, AssetFreezeTransactionBuilder, Byte32, TransactionHeaderBuilder}; | ||
| use base64::{prelude::BASE64_STANDARD, Engine}; | ||
|
|
||
| pub struct AssetFreezeTransactionMother {} |
There was a problem hiding this comment.
test data builder for asset freeze are refactored to another file, similar to https://github.com/algorandfoundation/algokit-core/blob/main/crates/algokit_transact/src/test_utils/asset_config.rs
| } | ||
|
|
||
| pub fn asset_unfreeze() -> AssetFreezeTransactionBuilder { | ||
| // testnet-LZ2ODDAT4ATAVJUEQW34DIKMPCMBXCCHOSIYKMWGBPEVNHLSEV2A |
There was a problem hiding this comment.
An asset unfreeze transaction is built with real data from testnet. This allows us to confirm that our encoding is correct.
| } | ||
|
|
||
| #[test] | ||
| fn test_asset_unfreeze_snapshot() { |
There was a problem hiding this comment.
Test snapshot to make sure that the encoding logic is correct.
| } | ||
|
|
||
| #[test] | ||
| fn test_asset_freeze_transaction_encoding() { |
There was a problem hiding this comment.
We don't need this test because the encoding logic is ready verified by the snapshot test. For decoding, I am not sure if we need tests for each transaction type because most of the heavy lifting is done by serde.
| } | ||
|
|
||
| #[test] | ||
| fn test_asset_unfreeze_transaction_encoding() { |
There was a problem hiding this comment.
Same as above, encoding is tested by real transaction data in the snapshot test.
| } | ||
|
|
||
| #[test] | ||
| fn test_asset_freeze_real_transaction_ids() { |
There was a problem hiding this comment.
This is already tested by the snapshot test
| } | ||
|
|
||
| #[test] | ||
| fn test_asset_freeze_required_fields() { |
There was a problem hiding this comment.
I think this isn't needed, it's testing the builder which is a 3rd party library. I'd trust that they have their own test suite & we don't need to test their logic.
| } | ||
|
|
||
| #[test] | ||
| fn test_asset_freeze_serialization_fields() { |
There was a problem hiding this comment.
Feels like this is testing serde
| #[serde(skip_serializing_if = "std::ops::Not::not")] | ||
| #[builder(default)] | ||
| pub frozen: Option<bool>, | ||
| pub frozen: bool, |
There was a problem hiding this comment.
Sorry I missed your comment earlier this week. The reason is was done this way is the frozen field is a required field for asset freeze transaction. However, only for encoding purposed, it's skipped if the value is False.
| !(v.is_null() | ||
| || v.is_boolean() | ||
| && v.as_bool() == Some(false) | ||
| && !BOOLEAN_FIELDS_TO_KEEP.contains(&k.to_case(Case::Snake).as_str()) |
There was a problem hiding this comment.
This is not needed, the defaultReviver in TypeScript and Python should set it to false if undefined.
2 participants
Fixes #
Proposed Changes