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

Trait implementation #1246

Merged
merged 98 commits into from Feb 21, 2020
Merged

Trait implementation #1246

merged 98 commits into from Feb 21, 2020

Conversation

lgalabru
Copy link
Member

@lgalabru lgalabru commented Feb 3, 2020

This PR is addressing #1168.

By doing so, we're introducing a mechanism for dynamically calling contracts while keeping our ability to run static analysis on contracts.
When a contract need to dispatch a call dynamically, it has to describe the behavior expected from the callee by defining a trait.

(define-trait can-transfer-tokens (
    (transfer-from? (principal principal uint) (response uint)))

(define-public (swap (token-a <can-transfer-tokens>) 
                     (amount-a uint) 
                     (owner-a principal)
                     (token-b <can-transfer-tokens>) 
                     (amount-b uint) 
                     (owner-b principal)))
     (begin
         (unwrap! (contract-call? token-a transfer-from? owner-a owner-b amount-a))
         (unwrap! (contract-call? token-b transfer-from? owner-b owner-a amount-b))))

Traits can be imported, aliased (for avoiding name collisions) and re-used:

(use-trait trait-a .contract-1.can-transfer-tokens) 

(define-public (pay-subscription (token-a <trait-a>))
    ...

Developers also have the option to explicitly declare that their contract is implementing a specific trait:

(impl-trait .contract-1.can-transfer-tokens) 

(define-fungible-token token)

(define-public (transfer-from? (sender principal) (recipient principal) (amount uint)) (response uint))
    (ft-transfer? token amount sender recipient))

In this case, the static analysis will throw if the contract is violating the specifications defined by the trait.

Copy link
Member

@jcnelson jcnelson left a comment

Choose a reason for hiding this comment

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

Thanks again for all your work on this @lgalabru! I think we still need to get some clarity on whether or not the system needs to be strict on requiring a contract to impl-trait, but I could be convinced. My only hesitation is from how it affects analysis and runtime cost. Maybe @kantai can comment?

src/vm/functions/mod.rs Outdated Show resolved Hide resolved
src/vm/callables.rs Outdated Show resolved Hide resolved
Copy link
Member

@kantai kantai left a comment

Choose a reason for hiding this comment

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

Great stuff, @lgalabru!

I left comments on the issues that I think need to be addressed. Otherwise, I think we should open an issue for moving the VM's trait check to be checked at the top-level contract-call execution (i.e. on transaction execution).

src/vm/types/signatures.rs Outdated Show resolved Hide resolved
Copy link
Member

@kantai kantai left a comment

Choose a reason for hiding this comment

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

Looks great, @lgalabru, thanks for addressing my feedback! I left one more comment, and otherwise LGTM!

I also opened an issue #1288 for moving the trait validation logic.

Copy link
Member

@jcnelson jcnelson left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @lgalabru! Just a small typo fix and you're good to go on my end.

@lgalabru lgalabru merged commit 6a57477 into master Feb 21, 2020
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.

None yet

5 participants