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

app-swift bindings #55

Merged
merged 21 commits into from
Aug 30, 2023
Merged

app-swift bindings #55

merged 21 commits into from
Aug 30, 2023

Conversation

krisbitney
Copy link
Contributor

@krisbitney krisbitney commented Aug 17, 2023

template PR: polywrap/wrap-cli#1870
related: #76

@krisbitney krisbitney requested a review from dOrgJelli as a code owner August 17, 2023 18:35
@dOrgJelli
Copy link
Contributor

In order to show the e2e developer experience, can we please create a template project that uses these bindings?

Similar to: https://github.com/polywrap/cli/blob/origin-dev/packages/templates/app/typescript/src/index.ts

Comment on lines 11 to 22
"as" | "break" | "const" | "continue" | "crate"
| "else" | "enum" | "extern" | "false" | "fn"
| "for" | "if" | "impl" | "in" | "let" | "loop"
| "match" | "mod" | "move" | "mut" | "pub" | "ref"
| "return" | "self" | "Self" | "static" | "struct" | "super"
| "trait" | "true" | "type" | "unsafe" | "use" | "where"
| "while" | "async" | "await" | "dyn" | "abstract"
| "become" | "box" | "Box" | "do" | "final" | "macro"
| "override" | "priv" | "typeof" | "unsized"
| "virtual" | "yield" | "try" | "macro_rules"
| "union" => true,
_ => false,
Copy link
Contributor

Choose a reason for hiding this comment

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

i think these do not apply to swift right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the keywords in the swift plugin and app bindings. Please take a look!

@@ -0,0 +1 @@
#import * from "wrap://ens/wraps.eth:abi-bindgen@0.1"
Copy link
Contributor

Choose a reason for hiding this comment

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

may we use wrapscan uri?

Copy link
Contributor Author

@krisbitney krisbitney Aug 30, 2023

Choose a reason for hiding this comment

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

For sure. What is it? The other bindings wraps aren't using it yet and I don't know how to get it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think @dOrgJelli was working on this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yah we'll do this in another PR, will do this today

Comment on lines 178 to 181
if (self.client == nil) {
self.client = BuilderConfig().addSystemDefault().addWeb3Default().build()
}
return self.client!
Copy link
Contributor

Choose a reason for hiding this comment

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

We should not use ! because its equivalent to unwrap() of a result in rust; I would to propose doing something like

if let client = self.client {
  return client
} else {
  let newClient = BuilderConfig()...
  self.client = newClient
  return newClient
}

this is more idiomatic imo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

Comment on lines 185 to 188
if (self.uri == nil) {
self.uri = try? Uri("testimport.uri.eth")
}
return self.uri!
Copy link
Contributor

Choose a reason for hiding this comment

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

since we are doing a try? here, handling this without ! would mean to add throws keyword in the definition of the function; hence, changing how the dev exp is... So let's keep it like this for now

noob q: this Uri should never be wrong the CLI would not create these bindings right? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, exactly. It is using the same URI used for the import in the polywrap.graphql schema, so if the URI were wrong then the app bindings wouldn't be generated in the first place.

Let me know if there is a better pattern you'd like to see here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the pattern to match the getDefaultClient pattern. Let me know your thoughts!


pub fn load_templates() -> Vec<Template> {
vec!(
Types_swift::load(),
Copy link
Contributor

Choose a reason for hiding this comment

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

is there any reason for having only the Types template? Would it be possible to also have the Module template so we can have things separately? Like in plugins

Copy link
Contributor Author

@krisbitney krisbitney Aug 30, 2023

Choose a reason for hiding this comment

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

I can separate these bindings into Module.swift and Types.swift.

I was just copying how the others are done. I think separating into more files is a great idea. To me, it would be ideal to have a separate file for each module.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@krisbitney krisbitney requested a review from cbrzn August 30, 2023 15:17
}
}

func getDefaultUri() -> Uri {
Copy link
Contributor

Choose a reason for hiding this comment

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

We also need getDefaultEnv method

Copy link
Contributor

Choose a reason for hiding this comment

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

Talked with Niraj, we can do this in another PR and release a new version on wrapscan (CLI will get updated automatically).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right that this is an oversight. I'm fixing this now.

@dOrgJelli dOrgJelli merged commit bc58e22 into wrap-0.1 Aug 30, 2023
12 checks passed
This was referenced Aug 30, 2023
@krisbitney krisbitney deleted the kris/swift-app-bindings branch August 31, 2023 15:25
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.

4 participants