-
Notifications
You must be signed in to change notification settings - Fork 196
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
feat(common,world): improvements for smart accounts #2578
Conversation
🦋 Changeset detectedLatest commit: 26410cf The changes in this PR will be included in the next version bump. This PR includes changesets to release 24 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@@ -31,6 +32,7 @@ export async function writeContract< | |||
>( | |||
client: Client<Transport, chain, account>, | |||
request: WriteContractParameters<abi, functionName, args, chain, account, chainOverride>, | |||
publicClient?: PublicClient<Transport, chain>, |
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 method is deprecated, so I didn't document this change in the changelog
I also don't love this pattern of adding an extra param, ideally should be an object here for easier extending, but again, this is deprecated so will leave it be
a future clean up should move this function inline into the transactionQueue
decorator
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.
opened an issue for this so we can add it to the backlog: #2584
.changeset/smooth-ducks-sell.md
Outdated
@@ -0,0 +1,9 @@ | |||
--- | |||
"@latticexyz/common": minor |
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.
so according to semver, this should be minor
because its an extra feature, not a fix
but maybe we don't want to trigger a bump to 2.1 yet?
@@ -0,0 +1,9 @@ | |||
--- | |||
"@latticexyz/common": patch |
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 should prob be minor but we're trying to batch a few minor version changes and don't want to block patch changes
writeArgs.address !== params.worldAddress || | ||
writeArgs.functionName === "call" || | ||
writeArgs.functionName === "callFrom" || | ||
writeArgs.functionName === "registerDelegationWithSignature" |
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.
Is this registerDelegationWithSignature
condition necessary? The registerDelegationWithSignature
function should be called by a delegator, not a delegatee. This callFrom
extension action is intended for the use of the delegatee's wallet client (after delegation), and the delegator's wallet client does not need to use this.
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.
registerDelegationWithSignature
is called by the delegatee, using a signed message by the delegator! This allows us to write to the world using only the delegatee/burner 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.
Make sense! Thanks.
pulled out of #2458