Skip to content

Conversation

@wjthieme
Copy link
Contributor

@wjthieme wjthieme commented Jan 23, 2025

introduced by #299. Tests did not catch this because the functions/types are behind a feature

  • Fixed the bug with imports not being resolved
  • Fixed an issue with errors from fetch functions not being converted
  • Added the fetch feature to the system example (with required optional dependencies)

@changeset-bot
Copy link

changeset-bot bot commented Jan 23, 2025

🦋 Changeset detected

Latest commit: ae52cc1

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@codama/renderers-rust Patch
@codama/renderers Patch

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

@wjthieme wjthieme changed the title bug: fetch return types not qualified properly bug: rust fetch return types not qualified properly Jan 23, 2025
Copy link
Contributor

@febo febo 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. A comment not directly related to this PR: would it make sense for the fetch_all_* variants to take a Vec<&Pubkey> since the fetch_ ones take a &Pubkey?

@wjthieme
Copy link
Contributor Author

wjthieme commented Jan 24, 2025

@febo I believe solana_client crate does &Pubkey for get_account and &[Pubkey] for get_multiple_accounts. Would it make sense to match that?

(Changing would probably be fine given this has never worked in a live version even though it would strictly be a breaking change)

@lorisleiva lorisleiva requested a review from febo January 27, 2025 13:55
febo
febo previously approved these changes Jan 27, 2025
@febo
Copy link
Contributor

febo commented Jan 27, 2025

@febo I believe solana_client crate does &Pubkey for get_account and &[Pubkey] for get_multiple_accounts. Would it make sense to match that?

(Changing would probably be fine given this has never worked in a live version even though it would strictly be a breaking change)

Agreed, let's match the API of solana_client.

@wjthieme
Copy link
Contributor Author

Agreed, let's match the API of solana_client.

Fixed!

@lorisleiva
Copy link
Member

Thank you! Just waiting on Febo's approval and LGTM.

Would you mind just rebasing the PR? Sorry I just merged another big PR and it created small conflicts.

@lorisleiva lorisleiva requested a review from febo January 28, 2025 10:07
@wjthieme wjthieme force-pushed the fix-rust-fetch-qualify branch from 2d144d9 to 479a3c8 Compare January 28, 2025 17:13
Copy link
Contributor

@febo febo 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, thanks!

@lorisleiva lorisleiva merged commit 18e75fe into codama-idl:main Jan 31, 2025
4 checks passed
@github-actions github-actions bot mentioned this pull request Jan 28, 2025
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