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

Update fungible token interface to check the type deposited in the deposit function #51

Merged
merged 5 commits into from
Apr 20, 2021

Conversation

joshuahannan
Copy link
Member

No description provided.

@joshuahannan joshuahannan requested a review from turbolent April 7, 2021 19:28
Comment on lines 188 to 189
from.getType().identifier.slice(from: 0, upTo: 18) == self.getType().identifier.slice(from: 0, upTo: 18):
"Cannot deposit an incompatible token type"
Copy link
Member

@turbolent turbolent Apr 7, 2021

Choose a reason for hiding this comment

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

This code does not match the comment, the code checks that the types are declared in the same account. Was that what you wanted to ensure?

If you really want to check that the type of the vault passes as an argument matches that of the vault that is accepting the deposit, you can just use self.isInstance and from.getType():

Suggested change
from.getType().identifier.slice(from: 0, upTo: 18) == self.getType().identifier.slice(from: 0, upTo: 18):
"Cannot deposit an incompatible token type"
from.isInstance(self.getType()):
"Cannot deposit an incompatible token type"

I've added a test case for this in onflow/cadence#779, see onflow/cadence@4f93166

It is als possible to just compare the types: from.getType() == self.getType():

Suggested change
from.getType().identifier.slice(from: 0, upTo: 18) == self.getType().identifier.slice(from: 0, upTo: 18):
"Cannot deposit an incompatible token type"
from.getType() == self.getType():
"Cannot deposit an incompatible token type"

Copy link
Member Author

Choose a reason for hiding this comment

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

oh yeah that makes a lot more sense! I'll change it to that. Thanks for the suggestion!

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in c84d060

@turbolent
Copy link
Member

@joshuahannan Updated the comment above

@joshuahannan
Copy link
Member Author

@turbolent Does it look ok now?

Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

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

Looks good!

What is the plan for releasing this? This change might break existing contracts, though to be fair, it should be rare and likely already a bug (forgetting to cast, accepting something else)

@joshuahannan
Copy link
Member Author

I'd like to upgrade it soon. I don't think it'll break any existing contracts because they should have already been doing the force cast anyway. But we might need to have a conversation with some other stakeholders to make sure this is ok.

@robmyers @dete Are you ok with this change to the interface?

@dete
Copy link

dete commented Apr 10, 2021

Yup! Seems like a correct addition to me.

@bluesign
Copy link

bluesign commented Apr 10, 2021

@joshuahannan
Copy link
Member Author

@bluesign yes, I think you're correct, though I am surprised that the tests didn't pick this up. The pre-condition should be in the Vault object instead of the Receiver interface. I'll update that

@bluesign
Copy link

bluesign commented Apr 11, 2021

@joshuahannan actually you have the correct implementation it seems. I had a total different imagination about forwarders, I was thinking they were implementing Vaults.

Also somehow I had wrong understanding about pre/post treat model, I was thinking it is against hostile contract vs hostile user.

@joshuahannan
Copy link
Member Author

yeah you're right. I thought I had put it in the receiver interface, but I had actually put it in the Vault definition, so we're all good.

@rheaplex
Copy link

This is a nice use of modern Cadence rtti. 👍

@joshuahannan joshuahannan merged commit 54218d0 into master Apr 20, 2021
@joshuahannan joshuahannan deleted the josh/type-check branch April 20, 2021 16:47
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.

5 participants