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

Idl: Limit account size to 60kb, allow closing idl accounts #2329

Merged
merged 16 commits into from
Jan 5, 2023

Conversation

ckamm
Copy link
Contributor

@ckamm ckamm commented Dec 21, 2022

Originally by @microwavedcola1

@vercel
Copy link

vercel bot commented Dec 21, 2022

@microwavedcola1 is attempting to deploy a commit to the coral-xyz Team on Vercel.

A member of the Team first needs to authorize it.

@Henry-E
Copy link
Contributor

Henry-E commented Dec 22, 2022

This looks nice, thanks for adding. @microwavedcola1 I don't know if you would mind adding a typescript test demonstrating the new close functionality to the existing set of IDL tests.

https://github.com/coral-xyz/anchor/blob/master/tests/anchor-cli-idl/tests/idl.ts

Also as with the other PRs

  • don't forget to check cargo clippy
  • please add a changelog with this as a new features

Thanks for all the great PRs! They all look like very nice additions to anchor, looking forward to merging.

@Henry-E
Copy link
Contributor

Henry-E commented Dec 26, 2022

I played around with adding accounts larger than 10kb (by adding a realloc / resize account function) and it seems like adding bytes to write buffer causes a memory issue before it can even reach account sizes above 10kb. So unless someone wants to switch over all the old IDL code to using zero copy accounts, this is probably as big as the account can be for now.

Not to mention, one issue with updating the IDL code to newer account types is that account types like Account rely on having the owner attribute implemented. But because the IDL accounts structs have their macros expanded while still in the anchor crate, then there's no way to know what the program ID to use is. Again, unless someone wants to port all of this code to being stored in a quote stream that only gets executed at the downstream program's compile time.

Anyway, I just need to

  • Fix the test (already added but it's failing silently somehow in the CI, just add a wait timer between tests to allow the validator to catch up)
  • Fix the clippy errors
  • update the change log

And then it's ready to merge i think

@Henry-E Henry-E changed the title Idl: Limit account size to 10kb, allow closing idl accounts Idl: Limit account size to 60kb, allow closing idl accounts Jan 4, 2023
@Henry-E
Copy link
Contributor

Henry-E commented Jan 4, 2023

Weird compile error happening with some programs but not others. It's possible that it's linked to the use of try_into() that was added in a number of places. Maybe there's a more explicit casting we can do instead? I read through a couple of the passing vs. failing programs and it's not clear what differences there are between them that could be causing this (i.e. they were both essentially empty/placeholder programs).

error[E0599]: no method named `try_into` found for type `u32` in the current scope
   --> programs/multiple-suites-run-single/src/lib.rs:5:1
    |
5   | #[program]
    | ^^^^^^^^^^ method not found in `u[32](https://github.com/coral-xyz/anchor/actions/runs/3840033520/jobs/6538648897#step:10:33)`
    
    error[E0599]: no method named `try_into` found for type `usize` in the current scope
   --> programs/multiple-suites-run-single/src/lib.rs:5:1
    |
5   | #[program]
    | ^^^^^^^^^^ method not found in `usize`
    |

edit: Mostly sure this is due to no including fully qualified paths for try_into. Will add them in now.

@Henry-E Henry-E merged commit 27bb695 into coral-xyz:master Jan 5, 2023
dovahcrow pushed a commit to dovahcrow/anchor that referenced this pull request Jan 13, 2023
crispheaney pushed a commit to drift-labs/anchor that referenced this pull request May 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants