Switch to the typed variants in scrypto lib
#1782
Conversation
|
Docker tags |
Benchmark for 3caa17cClick to view benchmark
|
lrubasze
force-pushed
the
feature/non_fungible_and_fungible_types
branch
2 times, most recently
from
e50c4a4
to
457693b
Compare
Just implement them for ResourceManager and be able to mark them as deprecated.
It breaks backward compatibility. However that shouldn't be a big deal since rust compiler nicely advises to use `into()` in order to convert fungible/non-fungible resource manager into generic resource manager.
lrubasze
force-pushed
the
feature/non_fungible_and_fungible_types
branch
from
457693b
to
a324be3
Compare
lrubasze
changed the title
scrypto where possible
lrubasze
changed the title
scrypto where possiblescrypto lib
scrypto/src/component/stubs.rs
Outdated
| fn claim_xrd(&mut self, bucket: Bucket) -> Bucket; | ||
| fn stake_as_owner(&mut self, stake: FungibleBucket) -> FungibleBucket; | ||
| fn stake(&mut self, stake: FungibleBucket) -> FungibleBucket; | ||
| fn unstake(&mut self, stake_unit_bucket: FungibleBucket) -> FungibleBucket; |
There was a problem hiding this comment.
I believe that unstake and claim_xrd are not correct.
- Input to
unstakeis indeed fungible but the output is a non-fungible claim NFT. - Input to
claim_xrdis a non-fungible claim NFT and the output is fungible XRD.
There was a problem hiding this comment.
Right, I've mixed it with validator stake units 🤦
| fn start_unlock_owner_stake_units(&mut self, requested_stake_unit_amount: Decimal); | ||
| fn finish_unlock_owner_stake_units(&mut self) -> Bucket; | ||
| fn finish_unlock_owner_stake_units(&mut self) -> FungibleBucket; | ||
| fn apply_emission( |
There was a problem hiding this comment.
Not quite user facing but should apply_reward's bucket be a FungibleBucket?
scrypto/src/component/stubs.rs
Outdated
| fn lock_fee(&mut self, amount: Decimal); | ||
| fn lock_contingent_fee(&mut self, amount: Decimal); | ||
| fn deposit(&mut self, bucket: Bucket); | ||
| fn deposit_batch(&mut self, buckets: Vec<Bucket>); | ||
| fn withdraw(&mut self, resource_address: ResourceAddress, amount: Decimal) -> Bucket; | ||
| fn withdraw_non_fungibles( | ||
| &mut self, | ||
| resource_address: ResourceAddress, | ||
| resource_address: NonFungibleBucket, |
There was a problem hiding this comment.
Probably an accidental mistake, but this should remain to be a ResourceAddress.
There was a problem hiding this comment.
It looks like this is an issue with how the function signatures were overriden in update-bindgen.sh.
There was a problem hiding this comment.
I believe the return type should be NonFungibleBucket
scrypto/src/component/stubs.rs
Outdated
| @@ -178,14 +179,14 @@ extern_blueprint_internal! { | |||
| ) -> Bucket; | |||
| fn lock_fee_and_withdraw_non_fungibles( | |||
| &mut self, | |||
| amount_to_lock: Decimal, | |||
| amount_to_lock: NonFungibleBucket, | |||
There was a problem hiding this comment.
Ditto: seems to have completely changed the type here.
scrypto/src/component/stubs.rs
Outdated
| resource_address: ResourceAddress, | ||
| ids: Vec<NonFungibleLocalId>, | ||
| ) -> Bucket; | ||
| fn create_proof_of_amount(&self, resource_address: ResourceAddress, amount: Decimal) -> Proof; | ||
| fn create_proof_of_amount(&self, resource_address: FungibleProof, amount: Decimal) -> Proof; |
There was a problem hiding this comment.
Ditto: seems to have completely changed the type here.
scrypto/src/component/stubs.rs
Outdated
| fn create_proof_of_non_fungibles( | ||
| &self, | ||
| resource_address: ResourceAddress, | ||
| resource_address: NonFungibleProof, |
There was a problem hiding this comment.
Ditto: seems to have completely changed the type here.
scrypto/src/component/stubs.rs
Outdated
| @@ -239,15 +240,16 @@ extern_blueprint_internal! { | |||
| ) -> Global<MultiResourcePool>; | |||
| }, | |||
| { | |||
| fn contribute(&mut self, buckets: Vec<Bucket>) -> (Bucket, Vec<Bucket>); | |||
| fn contribute(&mut self, buckets: Vec<FungibleBucket>) | |||
| -> (FungibleBucket, Vec<FungibleBucket>); | |||
| fn redeem(&mut self, bucket: Bucket) -> Vec<Bucket>; | |||
There was a problem hiding this comment.
I think redeem should be a (FungibleBucket) -> Vec<FungibleBucket> since the input is the pool units (fungible) and the output is fungible resources.
There was a problem hiding this comment.
typo in update-bindings.sh reedem 🤦
scrypto/src/component/stubs.rs
Outdated
| fn redeem(&mut self, bucket: Bucket) -> Vec<Bucket>; | ||
| fn protected_deposit(&mut self, bucket: Bucket); | ||
| fn protected_deposit(&mut self, bucket: FungibleBucket); | ||
| fn protected_withdraw( | ||
| &mut self, | ||
| resource_address: ResourceAddress, |
There was a problem hiding this comment.
Do we want to double down on the whole fungible and non-fungible bit and make this a FungibleResourceManager?
There was a problem hiding this comment.
Done.
As agreed refactored other parts, where ResourceAddress appears.
Switched to FungibleResourceManager, NonFungibleResourceManager or ResourceManager depending on context.
scrypto/src/component/stubs.rs
Outdated
| @@ -274,14 +276,14 @@ extern_blueprint_internal! { | |||
| ) -> Global<OneResourcePool>; | |||
| }, | |||
| { | |||
| fn contribute(&mut self, bucket: Bucket) -> Bucket; | |||
| fn contribute(&mut self, bucket: FungibleBucket) -> FungibleBucket; | |||
| fn redeem(&mut self, bucket: Bucket) -> Bucket; | |||
scrypto/src/component/stubs.rs
Outdated
| fn contribute( | ||
| &mut self, | ||
| buckets: (FungibleBucket, FungibleBucket), | ||
| ) -> (FungibleBucket, Option<FungibleBucket>); | ||
| fn redeem(&mut self, bucket: Bucket) -> (Bucket, Bucket); |
| @@ -382,10 +387,13 @@ extern_blueprint_internal! { | |||
| fn quick_confirm_recovery_role_badge_withdraw_attempt(&mut self) -> Bucket; | |||
| fn cancel_primary_role_badge_withdraw_attempt(&mut self); | |||
| fn cancel_recovery_role_badge_withdraw_attempt(&mut self); | |||
| fn mint_recovery_badges(&mut self, non_fungible_local_ids: Vec<NonFungibleLocalId>) -> Bucket; | |||
| fn mint_recovery_badges( | |||
There was a problem hiding this comment.
This should return a NonFungibleBucket and not a fungible bucket since the recovery badge is non-fungible.
scrypto/src/component/stubs.rs
Outdated
| fn lock_fee(&mut self, amount: Decimal); | ||
| fn lock_contingent_fee(&mut self, amount: Decimal); | ||
| fn deposit(&mut self, bucket: Bucket); | ||
| fn deposit_batch(&mut self, buckets: Vec<Bucket>); | ||
| fn withdraw(&mut self, resource_address: ResourceAddress, amount: Decimal) -> Bucket; | ||
| fn withdraw_non_fungibles( | ||
| &mut self, | ||
| resource_address: ResourceAddress, | ||
| resource_address: NonFungibleBucket, | ||
| ids: Vec<NonFungibleLocalId>, | ||
| ) -> Bucket; |
There was a problem hiding this comment.
I think that this should return NonFungibleBucket.
scrypto/src/resource/auth_zone.rs
Outdated
| &self, | ||
| amount: A, | ||
| resource_address: ResourceAddress, | ||
| ) -> Proof; | ||
| ) -> FungibleProof; |
There was a problem hiding this comment.
Is this true? Is a proof created by amount always fungible? IIRC I don't think that this is correct. I think that a proof of non-fungibles can be created by amount. I think that this is one case where it would be good to add a test to ensure that this is true. A test for this could be: 1. Creating proofs of fungibles by amount works, 2. creating proofs of non-fungibles by amount does not work, therefore we can safely use a FungibleProof here.
There was a problem hiding this comment.
According to the sources one can create_proof_of_amount of non-fungibles if amount is None (see compose_proof_by_amount), which is never a case, since amount is wrapped with Some.
Thus, realistically, there is no chance to create a non-fungible proof of amount.
Agree, the best is to create a test that will always verify it.
There was a problem hiding this comment.
According to the sources one can create_proof_of_amount of non-fungibles if amount is None (see compose_proof_by_amount)
I was wrong, compose_proof_by_amount is able to return non-fungible proof if amount is Some. Will revert this
lrubasze
force-pushed
the
feature/non_fungible_and_fungible_types
branch
from
2ed034b
to
220268f
Compare
| fn lock_fee(&mut self, amount: Decimal); | ||
| fn lock_contingent_fee(&mut self, amount: Decimal); | ||
| fn deposit(&mut self, bucket: Bucket); | ||
| fn deposit_batch(&mut self, buckets: Vec<Bucket>); | ||
| fn withdraw(&mut self, resource_address: ResourceAddress, amount: Decimal) -> Bucket; | ||
| fn withdraw(&mut self, resource_address: ResourceManager, amount: Decimal) -> Bucket; |
There was a problem hiding this comment.
I was a bit shocked by the change, despite knowing that we're introducing ResourceManager, FungibleResourceManager and NonFungibleResourceManager.
Are we feeling comfortable with replacing resource_address with resource manager? Curious to learn how other people feel about this.
There was a problem hiding this comment.
It's interesting. If we're saying that a fungible and non fungible resources are FungibleResourceManager and NonFungibleResourceManager then it makes sense that an undecided resource would be a ResourceManager. But I see your point on this being a bit of a big change.
There was a problem hiding this comment.
Perhaps this is one where traits can be useful? As an example, with some changes to the extern_blueprint macro I imagine that we can support resource_address: impl Into<ResourceAddress> which seems like a more correct argument type for this function.
There was a problem hiding this comment.
I think in my mind there are two different things: addresses (identifiers) and stubs, similar to component address and Global<T>.
The question is more about if we should use address or stubs for the interfaces.
Stubs would make Scrypto-to-Scrypto interaction easier, as there is no more conversion needed and everything is type checked.
On the other side, manifest developers/readers will need to map resource manager stubs to addresses.
| fn contribute(&mut self, buckets: Vec<Bucket>) -> (Bucket, Vec<Bucket>); | ||
| fn redeem(&mut self, bucket: Bucket) -> Vec<Bucket>; | ||
| fn protected_deposit(&mut self, bucket: Bucket); | ||
| fn contribute(&mut self, buckets: Vec<FungibleBucket>) |
There was a problem hiding this comment.
I think there was some product requirement to support treating non-fungible in a fungible way. @0xOmarA what do you think?
There was a problem hiding this comment.
AFAIK pools of non-fungibles was not a product requirement and is actually something that the pools currently reject:
Summary
Use typed variants in
scryptowhere possible.Eg.
claim_royalties()returnFungibleBucketinstead ofBucketFungibleVaultmethods useFungibleBucketetc.
Testing
Tests have been updated.
Update Recommendations
For dApp Developers
This is breaking change. Compiler might complain about mismatched types. But in most cases it will also nicely provides a suggested change.
eg.