Move from ValueQuery to OptionQuery for pallet-manta-pay

[bhgomes]

Depending on the semantics we should see if we should move

[Apokalip]

@bhgomes Looking around the manta-pay code I really am not sure why would we want that, we are not using either the ValueQuery on Empty generic and neither do we have places where the code is written in a way that would require None checks, as most times it is checked for .contains(). If I am missing something or its for future changes, can you explain more please?

[bhgomes]

@bhgomes Looking around the manta-pay code I really am not sure why would we want that, we are not using either the ValueQuery on Empty generic and neither do we have places where the code is written in a way that would require None checks, as most times it is checked for .contains(). If I am missing something or its for future changes, can you explain more please?

@Apokalip This was from a discussion from a meeting, I think @Dengjianping brought this up. Not sure if it's something we have to do or not, but recording this issue to check if we need to change anything. Feel free to close it if you think it's not needed.

[Dengjianping]

@Apokalip @bhgomes I tried to get value by nonexistent key, from storages like Shards and VoidNumberSetInsertionOrder, but default value will be returned.

#[test]
fn test_default_storage() {
    new_test_ext().execute_with(|| {
        let shard = crate::Shards::<Test>::get(0, 2);
        dbg!(shard);
        let vn = crate::VoidNumberSetInsertionOrder::<Test>::get(5);
        dbg!(vn);
        assert!(!crate::VoidNumberSetInsertionOrder::<Test>::contains_key(5));
    });
}

Like this one, if the key is invalid, it still return value: https://github.com/Manta-Network/Manta/blob/manta/pallets/manta-pay/src/lib.rs#L568

I just feel a little concern about the default value in the future, it might cause unexpected behavior.

[Garandor]

Hey team! Please add your planning poker estimate with Zenhub @ghzlatarev @Dengjianping

[Garandor]

The fact that return Shards::<T>::contains_key(shard_index, idx); works as intended with a ValueQuery if the key does not exist is only obvious from rustdoc (https://docs.rs/frame-support/2.0.0-rc4/frame_support/storage/trait.StorageDoubleMap.html#tymethod.contains_key) but not from looking at the code. I agree that this is confusing and we run the danger of using get instead of try_get somewhere undetected.
I'd prefer we used OptionQueries for at least all StorageMap and StorageDoubleMap types:

• UtxoSet
• Shards
• ShardTrees
• UtxoAccumulatorOutputs
• NullifierCommitmentSet
• NullifierSetInsertionOrder

Basically all items except NullifierSetSize which is a simple StorageValue...

I'd estimate we have to substitute 50 occurences, but explicitly handling the Options should be simple at each occurrence.
Also, I don't think calamari would need a migration since it's just the query return type that changes, not the storage representation.

[Dengjianping]

I went through get operation for all storages in mantapay, like:
https://github.com/Manta-Network/Manta/blob/manta/pallets/manta-pay/src/lib.rs#L1008
https://github.com/Manta-Network/Manta/blob/manta/pallets/manta-pay/src/lib.rs#L664
https://github.com/Manta-Network/Manta/blob/manta/pallets/manta-pay/src/lib.rs#L880

Two of them are for getting number, it might be fine(default value for number is 0), but for line 1008, I'm not sure it's safe enough. At least do contains_key then get.

[Garandor]

Hey team! Please add your planning poker estimate with Zenhub @Apokalip @zqhxuyuan

[Garandor]

We got some spread on the vote which makes sense since we have no reference issues yet.
Lets discuss the extremes and then vote again unless we can quickly agree on a value

@Dengjianping pls explain why you chose 1
@ghzlatarev pls explain why you chose 5

Specifically estimate Effort to implement and uncertainty about possible problems encountered while doing so

[Dengjianping]

I found only few get operations on those ValueQuery storages, so the migration to OptionQuery, we might add some more error variants for error handling, and then write some test cases to cover those changes.
@Garandor

[Garandor]

I tend to agree that effort to implement is very low, but there's some uncertainty about potential breakage, which is why i did not choose 1.

@ghzlatarev your opinion?

[ghzlatarev]

Tend to be more conservative and that it will take longer than expected, so that's why i voted 5.