-
Notifications
You must be signed in to change notification settings - Fork 13
feat!: refactor lsig delegation #504
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
base: decoupling
Are you sure you want to change the base?
Conversation
|
|
||
| for (const prop in addrWithSigners) { | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| ;(acct as any)[prop] = (addrWithSigners as any)[prop] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is pretty gross but it's needed for backwards compatibility. Tbh I don't think we should have ever injected properties into an existing class, so maybe this could be a candidate for a breaking change.
3cf1fdb to
a350ce4
Compare
f8e8016 to
2448b22
Compare
2448b22 to
272e3b9
Compare
| import { MultisigAccount } from './multisig' | ||
| import { TransactionSigner } from './signer' | ||
| import { AddressWithDelegatedLsigSigner, TransactionSigner } from './signer' | ||
| import { LogicSignature, MultisigSignature, SignedTransaction, encodeSignedTransaction } from './transactions/signed-transaction' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should rename LogicSignature to LogicSigSignature to differentiate between LogicSig?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think that would make sense. Alternatively we could call this LogicSig LogicSigProgram but that would be more of a breaking change from algosdk
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think either works. LogicSigSignature would match nicely with MultisigSignature.
| accountGetter?: (algorand: AlgorandClient) => Promise< | ||
| Address & | ||
| AddressWithTransactionSigner & { | ||
| account: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should extract a type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is the only place this type is used and I have no idea what to call it lol, but not opposed. Since this is a test function maybe we could also make a breaking change to simplify the return type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
haha yeah, I don't have an good suggestions on naming.
Not opposed to making a breaking change. I think the accountGetter feature is probably not extensively utilised anyways.
Co-authored-by: Neil Campbell <neil.campbell@makerx.com.au>
a2a2846 to
f1354ef
Compare
This PR is a refactor of how lsig delegation works. There are three main changes
LogicSigclass that does NOT contain any signature (just program/args)LogicSigAccountis now tied to only one delegator. This ensures that the sameLogicSigAccountalways has the same address (which is now the delegator)lsigSignerfunction now returns the Address it is signing for and either thesigorlmsigThe driver for this change is to simplify the flow for delegation and properly separate different delegations for the same program.