-
Notifications
You must be signed in to change notification settings - Fork 21
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
Add fetch function helpers to accounts page #299
Conversation
🦋 Changeset detectedLatest commit: 560ebef The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
# Conflicts: # packages/renderers-rust/public/templates/accountsPage.njk # packages/renderers-rust/test/accountsPage.test.ts
Oh interesting, thanks for that! @febo any thoughts about shipping this? |
@febo if you could look at this that would be great! One thing I am not 100% sure about is that with this change every generated client will have their own version of |
This is very nice! You are right that by having the Perhaps what we could do is to define these types and maybe the logic for the |
Also, I think it might be more useful to use nonblocking rpc client instead for the fetch functions (the caller can make an async function sync by blocking or using a runtime but not the other way around). If that sounds good I can make that change! (we could also do both blocking and nonblocking in different sub-modules) |
I 100% think we need to have an external crate that is being used by the generated code to avoid code repetition. But I don't think this is Codama. This the equivalent of the new Web3.js for the JavaScript renderer. We need something like "Web3.rs" for the Rust renderer. In other words, a Rust client framework that includes something akin to codecs and helpers functions like the ones introduced in this PR. |
Nice, this is a good idea. @wjthieme Would you like to create this repo with the helpers and we can then use it on the renderers? The helpers will probably be independent of the Codama client, like, you could use them standalone in any application. Once this is available as a crate, we can update this PR to make use of that and avoid duplicating the types/logic. |
I'd be down to create this. Do you have a location in mind for the repo or doesn't really matter? |
I don't think it matters – happy to help with naming, etc. |
FYI, @joncinque and I recently talked about moving away from |
@lorisleiva moved the types to the |
Thanks @wjthieme! Could you run the tests locally so it generates the changes in the end-to-end tests and push the changes in a commit? Otherwise CI will fail. This will also help me see the generated changes when I review to double check everything is okay. We'll also need a |
@lorisleiva added the changeset and found that the sharedPage was never rendered anywhere in the rust client. I proposed a fix but wasn't fully sure when it should be rendered or not. Let me know if my implementation is correct! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just waiting for CI to pass but looks good otherwise, thanks!
@febo are you happy with this?
Thanks! Could you just re-run the tests locally and commit the changes so the e2e folders are updated as well? |
The |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! 🍺
Fetch helper functions are currently included in the js client but not in the rust client. This PR adds similar fetch helper functions to the rust renderer.
I've added it behind a feature flag because it requires solana-client and solana-sdk crates which you don't want for CPIs.
Let me know what you think!