-
Notifications
You must be signed in to change notification settings - Fork 225
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
wallet misc #6191
wallet misc #6191
Conversation
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.
👍
@@ -43,7 +44,7 @@ export const makeOfferExecutor = ({ | |||
onStatusChange, | |||
onNewContinuingOffer, | |||
}) => { | |||
const { invitationFromSpec, lastOfferId, purseForBrand } = powers; | |||
const { invitationFromSpec, lastOfferId, logger, purseForBrand } = powers; |
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.
👍 ocap-happy logging
const { | ||
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.
hm. I missed something somewhere. A wallet knows the address it's associated with? Is that necessary? It sets off my ACL spidey-sense... it feels vagely like caller or msg.sender
in solidity.
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 had the same tingle. It's not strictly necessary. We could take it out by passing two other things in instead:
- wallet logger
- wallet storageNode
But it has to persist through init and there may be some churn with durability, so I'll punt until then: #5894
numWantsSatisfied: numSatisfied, | ||
}); | ||
}, | ||
handleError, |
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.
The commit message doesn't mention this. (not critical)
Description
Security Considerations
Stops publishing offerResult for copyRecord. Publishes simple types.
Documentation Considerations
--
Testing Considerations
--