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

Added WithdrawWithheldTokensFromAccounts IX #3128

Merged

Conversation

deanmlittle
Copy link
Contributor

It seems WithdrawWithheldTokensFromAccounts ix was missing from token22 transfer fee extensions, so I implemented it.

You can check out the instruction here

It allows the fee withdraw authority to withdraw fees from t22 transfer fee extension accounts directly without having to first harvest to mint, then harvest from mint. Code is tested and working locally.

This unlocks the ability for Anchor programs to delegate a PDA signer as the token withdraw authority, drastically improving security, as any signer can invoke an IX to claim tokens to a treasury without having to first claim to a mint account.

Copy link

vercel bot commented Jul 28, 2024

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

A member of the Team first needs to authorize it.

Copy link
Collaborator

@acheroncrypto acheroncrypto left a comment

Choose a reason for hiding this comment

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

Thanks, could you note this feature in the CHANGELOG?

Added Changelog entry
@deanmlittle
Copy link
Contributor Author

deanmlittle commented Jul 28, 2024

Added a Changelog entry. FYI: I forked the v0.30.1 tag instead of master. Not sure if this is a standard way of making a PR, but the new discriminator changes in master were causing backwards compatibility issues with the CLI, so I wanted to isolate for that.

@acheroncrypto
Copy link
Collaborator

FYI: I forked the v0.30.1 tag instead of master.

It would be better to fork the branch you're PRing to (master in this case), otherwise CI won't work as expected.

the new discriminator changes in master were causing backwards compatibility issues with the CLI

Could you expand on this? I don't think the new custom discriminator changes should cause any compatibility issues for the CLI — in fact the CLI code hasn't changed at all to support it (14cec14...c5337c5).

There was a discriminator-related breakage from v0.29 -> v0.30 because of the new IDL spec (#2824) and required discriminator field, but there shouldn't be any backwards compatibility issue in CLI or IDL between v0.30.1 and latest.

@deanmlittle
Copy link
Contributor Author

deanmlittle commented Jul 28, 2024

If you import the current master into an anchor program using git in the Cargo toml and try to run anchor build using CLI 0.30.1, idl-build will fail due to a lifetime issue on the DISCRIMINATOR and recommend you change the lifetime to 'static.

@acheroncrypto
Copy link
Collaborator

Thanks, the reason for this could be that you haven't changed anchor-spl to git version, or there might be another dependency that depends on an older version of Anchor.

The CLI itself is unaware of the &'static [u8] type change introduced in #3098, as it just builds and runs the code coming from the codegen.

There will be a conflict in the history due to the CHANGELOG override, could you rebase this branch to master?

@deanmlittle
Copy link
Contributor Author

deanmlittle commented Jul 28, 2024

I do not believe that to be the case. I do not have any third-party packages, and am importing both lang and spl from my own branch forked from master which also implements the added ix above. There seems to be a genuine backwards compatibility issue there.

As this branch is currently being used in prod due the ix not being available in anchor yet, if you would like me to submit a version forked from master, I will have to create a new PR from that other branch that I initially forked from master, with the caveat that although the code would be identical, I cannot make the claim that it is tested and working locally with CLI v0.30.1 due to the aforementioned discriminator lifetime backwards-compatibility issue.

@deanmlittle
Copy link
Contributor Author

Found the culprit: https://github.com/coral-xyz/anchor/blob/master/spl/src/idl_build.rs#L13

If we replace &[u8] with &'static[u8] it builds with 0.30.1.

If you would like me to make a combination of these two fixes with this PR, I can do that.

@acheroncrypto
Copy link
Collaborator

Found the culprit: https://github.com/coral-xyz/anchor/blob/master/spl/src/idl_build.rs#L13

If we replace &[u8] with &'static[u8] it builds with 0.30.1.

Hm, what's your nightly version? The recent versions the nightly compiler, which we use to build the IDL, should resolve elided lifetimes of associated constants to static if no other lifetimes are in scope. See rust-lang/rust#125190

Repro:

trait Discriminator {
    const DISCRIMINATOR: &'static [u8];
}

struct MyStruct;

impl Discriminator for MyStruct {
    const DISCRIMINATOR: &[u8] = &[];
}
cargo b

fails with rust-lang/rust#115010 (stable), but

cargo +nightly b

compiles.

If you would like me to make a combination of these two fixes with this PR, I can do that.

It wouldn't hurt to annotate the static lifetime, but since the changes are unrelated, it would be better if you could create a separate PR for the lifetime change.

@deanmlittle
Copy link
Contributor Author

Will try nightly and report back shortly. Your explanation makes sense. Sounds like it is a good idea to make that PR anyway so will do that first then rebase this from updated master.

@deanmlittle
Copy link
Contributor Author

Will rebase from here here once merged

@deanmlittle
Copy link
Contributor Author

I have rebased to current master if you wanted to merge this first. The two shouldn't conflict anyway.

Copy link
Collaborator

@acheroncrypto acheroncrypto left a comment

Choose a reason for hiding this comment

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

Thank you!

@acheroncrypto acheroncrypto merged commit a4b751c into coral-xyz:master Jul 28, 2024
48 of 49 checks passed
@deanmlittle deanmlittle deleted the withdraw-fees-from-accounts branch July 29, 2024 15:23
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.

2 participants