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

lang: Clearer error when handling not initialized accounts #1024

Merged
merged 1 commit into from
Nov 18, 2021

Conversation

cyphersnake
Copy link
Contributor

@cyphersnake cyphersnake commented Nov 15, 2021

Close #1015

  • Add ErrorCode::AccountNotInitialized
  • Add check in Account::try_from & Account::try_from_unchecked

@fanatid
Copy link
Contributor

fanatid commented Nov 15, 2021

Since this only more details errors and ProgramAccount / ProgramState are deprecated I do not think that we need a new trait.

@cyphersnake
Copy link
Contributor Author

@fanatid
I added this trait more for clarification of what I am checking and why, rather than for reuse in deprecated structs. I can clarify this in the comments to the if statement, but as for me this is more bad code practice, than rustdoc.

And to be honest, I just moved our internal trait to anchor, since this is how we check if the account is initialized or not. 😅
Add a reaction: 👍 to this post, if you consider it overkill and I'll remove it in favor of a simple use of if.

@cyphersnake cyphersnake force-pushed the master branch 3 times, most recently from b28e40a to 49679a4 Compare November 15, 2021 09:35
@cyphersnake
Copy link
Contributor Author

cyphersnake commented Nov 15, 2021

I see CI dropped on some tests, I'll figure it out and fix it a little later

lang/src/lib.rs Outdated Show resolved Hide resolved
@cyphersnake cyphersnake force-pushed the master branch 2 times, most recently from 3596b62 to 5c440d6 Compare November 15, 2021 13:38
lang/src/account.rs Outdated Show resolved Hide resolved
@fanatid
Copy link
Contributor

fanatid commented Nov 15, 2021

We need a record about a new error in CHANGELOG.md

@fanatid
Copy link
Contributor

fanatid commented Nov 15, 2021

It would be nice if we will have a test somewhere for a new error.

@cyphersnake cyphersnake force-pushed the master branch 2 times, most recently from 66d95d6 to 2cbd973 Compare November 15, 2021 17:39
@cyphersnake cyphersnake requested a review from fanatid November 15, 2021 17:40
@@ -13,7 +13,11 @@ incremented for features.

### Fixes

lang: Add `deprecated` attribute to `ProgramAccount` ([#1014](https://github.com/project-serum/anchor/pull/1014)).
* lang: Add `deprecated` attribute to `ProgramAccount` ([#1014](https://github.com/project-serum/anchor/pull/1014)).
Copy link
Contributor

Choose a reason for hiding this comment

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

🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the whole CHANGELOG, you don't have single items without a "*" at the beginning. I thought it was a typo and corrected it. Boy Scout Rule 😅

@fanatid
Copy link
Contributor

fanatid commented Nov 15, 2021

LGTM, thanks for the test!

@armaniferrante
Copy link
Member

The newly added test fails.

@cyphersnake
Copy link
Contributor Author

cyphersnake commented Nov 16, 2021

The newly added test fails.

Fixed. CI passed

@armaniferrante Is there a way to immediately run tests with updated clients in JS? Or I just need to learn how repos CI do it?

UPD:
@fanatid I understand that most likely it is possible to link a specific js client, cli and everything together and only then run the tests, however, it would be nice to have a command that can do this completely by itself (and without affecting stable version on the env). Through cargo make for example or something similar for yarn. As a last resort via Dockerfile. 49 minutes in CI doesn't seem like the main way to debug an application.

@cyphersnake
Copy link
Contributor Author

@armaniferrante Do I still need to improve PR somehow? Let me know please

@armaniferrante armaniferrante merged commit 2d44654 into coral-xyz:master Nov 18, 2021
@armaniferrante
Copy link
Member

Is there a way to immediately run tests with updated clients in JS? Or I just need to learn how repos CI do it?

I alias the path to my local CLI build and either yarn link or set the file path to the local ts build in the test. Not clean, but it's easy and gets the job done. Would be awesome to see future changes that handled this more elegantly.

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.

Feature Request: Clearer error when handling Account & ProgramAccount
3 participants