-
Notifications
You must be signed in to change notification settings - Fork 3
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
[BOOST-4786] feat(sdk,EventAction): add deriveActionClaimantFromTransaction method #194
Conversation
🦋 Changeset detectedLatest commit: 4b43ebc The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 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 |
params: ValidateActionStepParams, | ||
) { | ||
if (!('logs' in params)) { | ||
throw new ValidationLogsMissingError(); |
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.
We can remove this error since this was the only place it was used
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.
removed
knownSignatures?: Record<Hex, AbiEvent | AbiFunction>; | ||
abiItem?: AbiEvent | AbiFunction; |
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.
Awesome ty for adding function support
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.
ty for doing literally all the hard work and letting me ctrl+c your work
for (let log of decodedLogs) { | ||
if (!isAddressEqual(log.address, claimant.targetContract)) continue; | ||
let addressCandidate = this.validateClaimantAgainstArgs(claimant, log); | ||
if (addressCandidate) address = addressCandidate; | ||
} | ||
return address; |
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.
Thinking out loud here on an edge case here: would deriving claimant from event only work for txs with a single event? Cause if there are multiple event logs that match, it would basically just return the last match
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.
but I guess we can make sure that when defining an action claimant that the values must be strict!
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.
Thinking out loud here on an edge case here: would deriving claimant from event only work for txs with a single event? Cause if there are multiple event logs that match, it would basically just return the last match
yeah wasn't really sure how to handle that, @Quazia is this approach decent?
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.
🔥
lgtm, left a non-blocking comment
…, refactor ValidateActionStepParams slightly chore: address feedback
15d4a60
to
4b43ebc
Compare
🚨 Please review the guidelines for contributing to this repository.
Description
EventActions
's claimant configuration given a tx hash or logsValidateActionStepParams
to use event/function agnostic naming/typing and make accessing abi items off the params more consistent for action step validation as wellfix #139
💔 Thank you!