-
Notifications
You must be signed in to change notification settings - Fork 1
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
[ABW-3712] Allow to hide/unhide resources on Profile #199
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #199 +/- ##
======================================
Coverage 97.3% 97.4%
======================================
Files 912 921 +9
Lines 14604 14727 +123
Branches 64 64
======================================
+ Hits 14224 14346 +122
- Misses 373 374 +1
Partials 7 7
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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.
Good start!
sut.hideResource(kind: .nonFungible(.sample)) | ||
sut.hideResource(kind: .poolUnit(.sample)) | ||
|
||
XCTAssertEqual([.sampleOther, .sample], sut.hiddenResources.fungible) |
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.
I think I've used the other way around for all tests: lhs=actual, rhs=expected
Which is the "official" order, as set by Apple themselves, see eg test inside of XCTTest itself: https://github.com/swiftlang/swift-corelibs-xctest/blob/main/Tests/Functional/Observation/Selected/main.swift
#[derive( | ||
Clone, Debug, PartialEq, Eq, Hash, derive_more::Display, uniffi::Enum, | ||
)] | ||
pub enum ResourcePreferenceKind { |
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.
There is a macro you can use here! See the AddressOfAccountOrPersona, which is declared using address_union!(enum ResourcePreferenceKind: fungible, nonFungible, poolUnit);
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.
maybe enum AssetAddress
?
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.
there is no such thing as a FungibleAddress
(so it would represent fungibles as resource
), nor we want a NonFungibleResourceAddress
(we want NonFungibleGlobalId
), so not sure this address_union! macro helps us here.
pub struct ResourcePreferences { | ||
/// A dictionary detailing the user preferences for fungible resources. | ||
#[serde(default)] | ||
pub fungible: HashMap<ResourceAddress, EntityFlags>, |
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.
EntityFlag is wrong. The word "Entity" means "Account or Identity". At least in wallet context.
But taking one step back, what can you do more than hide? If nothing: we could use just IndexSet? Containing "hidden" addresses.
Since we are showing the different asset kinds in different sections in ui a simpler format would be to not introduce any new address union and just use:
struct AssetPreferences {
hidden_fungibles: IndexSet<ResourceAddress>,
hidden_pool_units: IndexSet<PoolUnitAddress>,
...
}
If we don't know of any planned other setting than hiding, this is a much more compact format and simpler.
But it is a bad format if we are indeed going to add support for another enum variant than "Hidden, NonHidden" eg: ConditionallyHiddenIfWorthMoreThan1000USd
.
Ok I'm leaning towards a new AssetPreferences enum.
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.
so it would be something like:
pub enum AssetVisibility {
Hidden,
// Just an example of a potential future variant
ConditionallyHiddenIfValueIsLessThan1000Usd
}
pub struct AssetPreference {
address: AssetAddress, // fungible | nonFungible | poolUnit
/// This does NOT allow for other flags, it is a mutually exclusive state.
visibility: AssetVisibility
}
struct AssetPreference {
assets: IndexMap<AssetAddress, AssetPreference>
}
Advantages:
- This retains insertion order of all preferences, "globally" (if ever interesting).
- Is more compact
- If we want preferences for more than visibility, we add a new enum for that, and a new field "next to"
visibility
.
This is also future proof, since we can always add more enum variants to AssetVisibility
and we can alway add new enum types and fiels to AssetPreference
. So... should work?
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.
Agree that maybe EntityFlags
aren't a great representation of "flags for an address". However, the reason I used them, is because if we ever extend the configuration of what is defined for entities, it probably applies for assets as well.
Of course, this is all based on assumptions that we don't know yet how will they turn out. In your ConditionallyHiddenIfValueIsLessThan1000Usd
, then it definitely makes sense to have the new resource configuration using a new preference definition (as the one you suggested). On my case, I thought a good example could have been "marking a resource/persona/account as favorite". In such case, a new EntityFlag::IsFavorite
would fit for all of them.
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.
Shared variants is not the same as semantically the same enum :) they should be able to evolve independently of each other. Also DeletedByUser
is the intention of account, they dont want it. Initially we gave the button the name Delete, then changed to Hide. Whereas "Hide asset" feature truely is a hide, since it is stored in the immutable Ledger - thus never can be deleted! So it should be named WhatIsTheNameOfTheEnum::Hidden
and not ::Deleted
.
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.
but with that argument hidden accounts/personas should also be named ::Hidden
since they aren't deleted from Ledger
pub struct ResourcePreferences { | ||
/// A dictionary detailing the user preferences for fungible resources. | ||
#[serde(default)] | ||
pub fungible: HashMap<ResourceAddress, EntityFlags>, |
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.
I think fungibles
, plural, no? It is a HashMap with settings for fungibles? (Or strictly: fungible assets plural)
|
||
impl ResourcePreferences { | ||
pub fn get_hidden_resources(&self) -> HiddenResources { | ||
let fungible = self |
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.
You can probably use a closure unlined here, being a function you reuse for the three collections (Rust does not allow nested functions, but you can use closure).
pub struct ResourcePreferences { | ||
/// A dictionary detailing the user preferences for fungible resources. | ||
#[serde(default)] | ||
pub fungible: HashMap<ResourceAddress, EntityFlags>, |
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.
You can use IndexMap to retain insertion order!
is_cloud_profile_sync_enabled, | ||
is_developer_mode_enabled | ||
is_developer_mode_enabled, | ||
is_advanced_lock_enabled | ||
)] | ||
pub struct Security { | ||
pub is_cloud_profile_sync_enabled: bool, |
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.
I would probably start using new types around bools now, easy to accidentally pass the wrong bool to the wrong value otherwise
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.
sorry, couldn't get what you meant here
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.
I mean we should change from:
pub struct Security {
...
pub is_cloud_profile_sync_enabled: bool,
pub is_developer_mode_enabled: bool,
pub is_advanced_lock_enabled: bool,
...
}
To:
#[derive(..., )]
#[serde(transparent)]
pub struct IsCloudSyncEnabled(bool);
#[derive(..., )]
#[serde(transparent)]
pub struct IsDeveloperModeEnabled(bool);
pub struct IsAdvancedLockEnabled(bool);
uniffi::custom_newtype!(IsCloudSyncEnabled, bool);
uniffi::custom_newtype!(IsDeveloperModeEnabled, bool);
...
pub struct Security {
...
pub is_cloud_profile_sync_enabled: IsCloudSyncEnabled,
pub is_developer_mode_enabled: IsDeveloperModeEnabled,
pub is_advanced_lock_enabled: IsAdvancedLockEnabled,
...
}
Making everything type safe :) this is using the "new type" paradigm, like Tagged
package in by Pointfree
(we seems suitable to use a macro for it perhaps! decl newtype_bool!(IsDeveloperModeEnabled);
...
#[display("assets: {:#?}", self.assets)] | ||
pub struct AssetPreferences { | ||
#[serde(default)] | ||
pub assets: HashMap<AssetAddress, AssetPreference>, |
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.
should be IndexMap
else we loose insertion order.
Otherwise might be worth doing like we do with most collection: let AssetAddress
impl Identifiable
and use the macro to declare a collection of AssetPreferences
plural. And use that here.
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.
oh right this whole thing should probably be decl_identified_vec_of(AssetPreference)
which creates a AssetPreferences
which is serde transparent, so it will be []
in JSON. Which is pretty nice.
} | ||
|
||
impl Hash for AssetPreferences { | ||
fn hash<H: Hasher>(&self, state: &mut H) { |
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.
and using the IdentifiedVec
will allow you to derive Hash
instead of manually implementing it.
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.
Nice, getting there! I would add doc an all new types and pub methods
src/core/types/bool_type.rs
Outdated
|
||
#[test] | ||
fn default() { | ||
assert!(ExampleTrue::default().0); |
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.
If you implement Deref
then you can write *sut
instead of sut.0
which imo is a bit cleaner
} | ||
|
||
impl AssetPreference { | ||
pub fn set_visibility(&mut self, visibility: AssetVisibility) { |
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.
Probably no need for a setter method. Can use pub field directly
x.set_visibility(AssetVisibility::Hidden) | ||
}) { | ||
if !self | ||
.update_with(asset.id(), |x| x.visibility = AssetVisibility::Hidden) |
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.
I THINK you can use write update_with(asset, ..
?
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.
I would need to do update_with(asset.clone(), ..)
in such case since I would be borrowing the asset (which is used in next line if update fails). So guess it is cheaper to leave as is?
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.
Update with takes an impl Borrow. I thought it auto borrowed, but im probably mistaken
@@ -46,6 +46,13 @@ | |||
}, | |||
"transaction": { | |||
"defaultDepositGuarantee": "0.975" | |||
}, | |||
"resource": { |
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.
this vectors needs to be updated right? @matiasbzurovski
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.
good catch! actually I am gonna revert this change since this JSON isn't used to validate the AssetsPreferences
and its corresponding test can deal without any assets
entry on the JSON.
* Add kt extensions for resource preferences * Remove packaging option * Update asset preferences kt extensions
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! Good job! Congratulations on your first macro!
Changelog
ResourcePreferences
(stored underProfile.AppPreferences
) that will allow user to hide fungible tokens, non-fungible tokens & pool units. ZeplinSecurity
to store new flag to indicate if advanced lock is required.