Skip to content

lang: Clearer error when handling not initialized accounts#1024

Merged
armaniferrante merged 1 commit into
otter-sec:masterfrom
debridge-finance:master
Nov 18, 2021
Merged

lang: Clearer error when handling not initialized accounts#1024
armaniferrante merged 1 commit into
otter-sec:masterfrom
debridge-finance:master

Conversation

@cyphersnake

@cyphersnake cyphersnake commented Nov 15, 2021

Copy link
Copy Markdown
Contributor

Close #1015

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

@fanatid

fanatid commented Nov 15, 2021

Copy link
Copy Markdown
Contributor

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

@cyphersnake

Copy link
Copy Markdown
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

cyphersnake commented Nov 15, 2021

Copy link
Copy Markdown
Contributor Author

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

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

fanatid commented Nov 15, 2021

Copy link
Copy Markdown
Contributor

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

@fanatid

fanatid commented Nov 15, 2021

Copy link
Copy Markdown
Contributor

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
Comment thread CHANGELOG.md
### 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
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🤔

Copy link
Copy Markdown
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 😅

Comment thread tests/errors/tests/errors.js Outdated
@fanatid

fanatid commented Nov 15, 2021

Copy link
Copy Markdown
Contributor

LGTM, thanks for the test!

@armaniferrante

Copy link
Copy Markdown
Contributor

The newly added test fails.

@cyphersnake

cyphersnake commented Nov 16, 2021

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
Contributor Author

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

@armaniferrante
armaniferrante merged commit 2d44654 into otter-sec:master Nov 18, 2021
@armaniferrante

Copy link
Copy Markdown
Contributor

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.

Otter-0x4ka5h pushed a commit to Otter-0x4ka5h/anchor that referenced this pull request Mar 25, 2026
@jamie-osec jamie-osec added this to the Pre-1.0 milestone Jun 24, 2026
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

4 participants