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

Make all logic implemented inside PSP22Data struct #2

Merged
merged 8 commits into from
Oct 3, 2023
Merged

Conversation

h4nsu
Copy link
Member

@h4nsu h4nsu commented Oct 2, 2023

Moves all PSP22 token logic from the example implementation (lib.rs) inside the PSP22Data struct. Thanks to that the logic is now fixed and a contract developer can use our implementation by simply implementing PSP22 trait in a "pass-through" way:

impl PSP22 for MyContract {
    ...
    #[ink(message)]
    fn balance_of(&self, owner: AccountId) -> u128 {
        self.data.balance_of(owner)
    }
    ...
    #[ink(message)]
    fn transfer(&mut self, to: AccountId, value: u128, data: Vec<u8>) -> Result<(), PSP22Error> {
        let events = self.data.transfer(self.env().caller(), to, value, data)?;
        self.emit_events(events); 
        Ok(())
    }
    ...
}

This approach:
a) greatly reduces the amount of boilerplate code that needs to be copied into user's contract
b) avoids using complicated macro on top of ink macro
c) (not sure if this is good, but OpenBrush had it) gives the user control over emitting events

Copy link
Member

@pmikolajczyk41 pmikolajczyk41 left a comment

Choose a reason for hiding this comment

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

Delegation seems to be clean solution (although might be a bit onerous for rich traits), so approving with a few minor comments.

There is one hidden thing that we'd better be careful about (especially when adapting this approach for other PSPs). The default implementation on the PSP22Data type should meet the assumption that no storage change is done if the method returns Err(_). A situation when this might lead to a big problem is such a custom delegation:

fn method(args) {
    if let Err(Event::MinorProblem) = self.data.method(args) {
        self.do_some_custom_fallback()
    }
}

In other words, we can't assume that a failed 'template' method will lead to a failed call. As far as I can see, your code meets this requirement, so we are ok.

lib.rs Show resolved Hide resolved
lib.rs Show resolved Hide resolved
@@ -24,6 +25,23 @@ mod token {
data: PSP22Data::new(supply, Self::env().caller()),
}
}

fn emit_events(&self, events: ink::prelude::vec::Vec<PSP22Event>) {
Copy link
Member

Choose a reason for hiding this comment

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

sad thing is that this method has to be copied for every psp22 contract... maybe we could extract it from here and expose somehow for reuse?

if you go with the other suggestion, you might end up with a very generic, convenient method for emitting arbitrary event iterable from a contract, which sounds nice :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately it's probably not possible to extract a method which uses self.env() outside the contract. Or maybe technically it's possible, but results in some templating nightmare.

This method is here temporary, after ink! allows defining events outside contracts, it will be gone and all self.emit_events(events) will be replaces with for e in events { self.env().emit_event(e); }

Copy link
Contributor

@deuszx deuszx Oct 3, 2023

Choose a reason for hiding this comment

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

I think you could extract (or rather implement in the PSP22Data) something along these lines:

 fn emit_event<C, E, EE>(emitter: EE, event: E) 
 where C: ContractEventBase,
             E: Into<<C as ContractEventBase>::Type>,
             EE: EventEmitter<C> {
            emitter.emit_event(event);
        }

@deuszx
Copy link
Contributor

deuszx commented Oct 3, 2023

Delegation seems to be clean solution (although might be a bit onerous for rich traits), so approving with a few minor comments.

There is one hidden thing that we'd better be careful about (especially when adapting this approach for other PSPs). The default implementation on the PSP22Data type should meet the assumption that no storage change is done if the method returns Err(_). A situation when this might lead to a big problem is such a custom delegation:

fn method(args) {
    if let Err(Event::MinorProblem) = self.data.method(args) {
        self.do_some_custom_fallback()
    }
}

In other words, we can't assume that a failed 'template' method will lead to a failed call. As far as I can see, your code meets this requirement, so we are ok.

Yes. In the previous PR I've suggested creating a "contract (not Smart Contract) level integration tests" - #1 (review) . It would cover the cases you're describing here.

data.rs Outdated
Comment on lines 8 to 19
pub enum PSP22Event {
Transfer(Option<AccountId>, Option<AccountId>, u128),
Approval(AccountId, AccountId, u128),
}

#[ink::storage_item]
#[derive(Debug, Default)]
pub struct PSP22Data {
total_supply: u128,
balances: Mapping<AccountId, u128>,
allowances: Mapping<(AccountId, AccountId), u128>,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It'd be good to have comments (///) on pub elements. Especially this is supposed to be an exemplary usage.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll add nice comments and extensive readme in a different PR

data.rs Outdated Show resolved Hide resolved
data.rs Outdated Show resolved Hide resolved
lib.rs Outdated Show resolved Hide resolved
@@ -0,0 +1,170 @@
use crate::PSP22Error;
Copy link
Contributor

Choose a reason for hiding this comment

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

We might think about adding a mod-level documentation here, explaining why this representation and how it's supposed to be integrated into other contracts.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will be added soon (tm) ;)

data.rs Show resolved Hide resolved

pub fn transfer(
&mut self,
caller: AccountId,
Copy link
Contributor

@deuszx deuszx Oct 3, 2023

Choose a reason for hiding this comment

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

Suggested change
caller: AccountId,
from: AccountId,

At this level of abstraction there's no notion of "caller" anymore and in this particular method, from == caller.

Copy link
Member Author

Choose a reason for hiding this comment

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

These internal methods of PSP22Data differ in signature form the ones defined by the PSP22 trait. My idea was to make it explicit, that this argument is meant to always be self.env().caller() when someone is implementing the trait.

@h4nsu h4nsu merged commit b2dddac into main Oct 3, 2023
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.

3 participants