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: Add Owner & Discriminator implementation for ix structures #2085

Merged
merged 1 commit into from Nov 24, 2022

Conversation

cyphersnake
Copy link
Contributor

@cyphersnake cyphersnake commented Jul 23, 2022

lang: Add into Discriminator trait constant DISCRIMINATOR
So that during match instructions or other entities there is no explicit instruction call of discriminator()
lang: Add Owner impl to instructions

Close #1994

@vercel
Copy link

vercel bot commented Jul 23, 2022

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

A member of the Team first needs to authorize it.

@vercel
Copy link

vercel bot commented Jul 25, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
anchor-docs ✅ Ready (Inspect) Visit Preview Jul 25, 2022 at 4:52PM (UTC)

@Henry-E
Copy link
Contributor

Henry-E commented Nov 22, 2022

Just checking that you have allow edits from maintainers active. I wanted to see whether we could include this PR in the next version by testing it localing and pushing some changes but I haven't been able to push changes to the PR fork.

@cyphersnake
Copy link
Contributor Author

@Henry-E This is not available on company forks. Can you tell me what changes I should make? If they are complex, then I could do the same PR from my personal account

@Henry-E
Copy link
Contributor

Henry-E commented Nov 23, 2022

It looks good as is, I also wondered why discriminator wasn't a trait constant.

I just wanted to fix the merge conflicts but if you wouldn't mind doing that, that would be great!

Random questions would be

  • why add an owner trait to the instructions?
  • The raw sig hash bytes for the accounts still gets used in some other places, e.g. the anchor_lang::AccountDeserialize / Serialize traits. Is it worth changing it to use the new constant there as well? Or would adding another trait bound on those traits be too annoying.
  • Because most of this change really takes place in the macro code it shouldn't be a breaking change for anyone except for people who are manually implementing their own traits, right?

lang: Add into `Discriminator` trait constant `DISCRIMINATOR`

So that during match instructions or other entities there is no explicit instruction call of `discriminator()`

lang: Add `Owner` impl to instructions
@cyphersnake
Copy link
Contributor Author

cyphersnake commented Nov 23, 2022

  1. In general, this PR was the first part of the changes that improve the rust-client experience. To do this, each type denoting a instruction must be self-sufficient in terms of context. We need to associate with the program id (owner trait) and with the connected accounts. In our fork, this issue was resolved a couple of months ago, but the whole chain of these changes is stuck by this PR.
    Initially, I came up with these changes because I wanted to generate a match (ix.program_id, ix.data) { (ix::OWNER, ix::DISCRIMINATOR) => todo!(), ... } for provided instruction set in offchain event-reader, but I was afraid to make changes to two base anchor traits at one time (Owner can also have a constant and be used in match) and started with one of them.

  2. I think removing dynamic discriminator counting everywhere is a valid idea, but this PR was only aimed at instructions. In the following I can correct in other places, I have already figured out the anchor-macros 😅

  3. Yes, only manual impl of these traits can break something for users. However, if our company is not alone in a Rust services practice, then it would be logical to put this in the Breaking column

@Henry-E
Copy link
Contributor

Henry-E commented Nov 24, 2022

Cool, this all sounds good. Do you reckon it's ready for merge now?

@cyphersnake
Copy link
Contributor Author

@Henry-E ready for 5 months 😏

@Henry-E
Copy link
Contributor

Henry-E commented Nov 24, 2022

Let send it then!

@Henry-E Henry-E merged commit a73bd72 into coral-xyz:master Nov 24, 2022
Henry-E pushed a commit to Henry-E/anchor that referenced this pull request Dec 6, 2022
…oral-xyz#2085)

lang: Add into `Discriminator` trait constant `DISCRIMINATOR`

So that during match instructions or other entities there is no explicit instruction call of `discriminator()`

lang: Add `Owner` impl to instructions

Co-authored-by: Mikhail Gorbachev <m.gorbachev@joinsprouttherapy.com>
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.

Add implementation of Discriminator & Owner to instruction
2 participants