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 return type to dictionary for discoverability of vault types #127

Merged
merged 3 commits into from
Mar 16, 2023

Conversation

satyamakgec
Copy link
Contributor

Extension to #123

contracts/FungibleToken.cdc Outdated Show resolved Hide resolved
lib/js/test/mocks/contracts/Token.cdc Outdated Show resolved Hide resolved
@joshuahannan joshuahannan merged commit e658295 into master Mar 16, 2023
@joshuahannan joshuahannan deleted the flip-69-implementation branch March 16, 2023 18:30
@bluesign
Copy link

bluesign commented Mar 17, 2023

what is the motivation here ? @satyamakgec

@joshuahannan
Copy link
Member

Dictionaries are more usable than arrays because you can index into them without having to iterate through the array to find an id, and you can still get an array from them anyway if you want by using dictionary.keys

@bluesign
Copy link

bluesign commented Mar 20, 2023

Dictionaries are more usable than arrays because you can index into them without having to iterate through the array to find an id, and you can still get an array from them anyway if you want by using dictionary.keys

But in this context it is only used for if item exists, which is Array.Contains in Cadence.

getSupportedVaultTypes().Contains(t)

vs

getSupportedVaultTypes()[t]!=nil

or even

if let exists = getSupportedVaultTypes(t){
   if exists {
  }
}

somehow I feel it doesn't make sense here. It would have some saving if it is very big dictionary maybe, but here I think readability is better trade off.

@joshuahannan
Copy link
Member

array.contains still has to iterate through the array under the hood to find the value, right? That is a O(n) operation, but with a dictionary, it is an O(1) operation I believe.

@bluesign
Copy link

bluesign commented Mar 20, 2023

array.contains still has to iterate through the array under the hood to find the value, right? That is a O(n) operation, but with a dictionary, it is an O(1) operation I believe.

yeah but it matters only for big N; here we have something like N<10 maybe on worst case ( on most cases N=1 ) ( even iterating can be cheaper for some N < x ) But I think in the end, readability vs cost tradeoff in a way.

PS: actually as the current state of Cadence, I think this makes sense ( with broken contracts etc ) but i believe Cadence should fix that

@joshuahannan
Copy link
Member

but it would matter for something like the NFT standard, and I want to make the function signatures as similar as possible for these kinds of methods so using them can be similar. We're going to add something like this for NFTs also

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