-
-
Notifications
You must be signed in to change notification settings - Fork 44
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
Chelonia in SW #2357
base: master
Are you sure you want to change the base?
Chelonia in SW #2357
Changes from 23 commits
9b89d48
bce3c5c
4601d99
15d5f86
2861605
80b1385
693173f
ba57fb4
13b4357
0a90ec4
22a0f6e
e5f8ed2
286570a
ab7d69d
3c78f54
c8a4441
17f3e77
fe4521a
870d9a6
001e813
48db75e
d99c1e0
90460e7
279e171
933957b
ce94156
bdd0f84
218dd9b
aa12e08
fd77e0b
18046a7
bbb7277
f7bc3d4
b08b48b
0f55f24
332e8cb
6fd370f
fe4e55d
ca116a5
e4f40bc
74b2dfa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,15 +21,16 @@ sbp('okTurtles.events/on', MESSAGE_RECEIVE_RAW, ({ | |
// If newMessage is undefined, it means that an existing message is being edited | ||
newMessage | ||
}) => { | ||
const getters = sbp('state/vuex/getters') | ||
const mentions = makeMentionFromUserID(getters.ourIdentityContractId) | ||
const state = sbp('chelonia/contract/state', contractID) | ||
const rootState = sbp('chelonia/rootState') | ||
const mentions = makeMentionFromUserID(rootState.loggedIn?.identityContractID) | ||
const msgData = newMessage || data | ||
const isMentionedMe = (!!newMessage || data.type === MESSAGE_TYPES.TEXT) && msgData.text && | ||
(msgData.text.includes(mentions.me) || msgData.text.includes(mentions.all)) | ||
|
||
if (!newMessage) { | ||
const isAlreadyAdded = !!getters | ||
.chatRoomUnreadMessages(contractID).find(m => m.messageHash === data.hash) | ||
// TODO: rootState.unreadMessages for SW | ||
const isAlreadyAdded = rootState.chatroom?.unreadMessages?.[contractID]?.unreadMessages.find(m => m.messageHash === data.hash) | ||
|
||
if (isAlreadyAdded && !isMentionedMe) { | ||
sbp('gi.actions/identity/kv/removeChatRoomUnreadMessage', { contractID, messageHash: data.hash }) | ||
|
@@ -42,10 +43,10 @@ sbp('okTurtles.events/on', MESSAGE_RECEIVE_RAW, ({ | |
messageHash: msgData.hash, | ||
height: msgData.height, | ||
text: msgData.text, | ||
isDMOrMention: isMentionedMe || getters.chatRoomAttributes.type === CHATROOM_TYPES.DIRECT_MESSAGE, | ||
isDMOrMention: isMentionedMe || state.attributes?.type === CHATROOM_TYPES.DIRECT_MESSAGE, | ||
messageType: !newMessage ? MESSAGE_TYPES.TEXT : data.type, | ||
memberID: innerSigningContractID, | ||
chatRoomName: getters.chatRoomAttributes.name | ||
chatRoomName: state.attributes?.name | ||
Comment on lines
+47
to
+50
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is preventing us from moving the getter definitions into a file that is shared by the frontend and the SW? That would be much preferable to this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Chatroom getters are a Vuex module, which are harder to emulate without Vuex because of state partitioning. In addition, those getters rely on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there nothing creative that's possible here? We can't define getters in a way that has them referring to the "the chatroom that corresponds to this i.e. "state paritioned getters" ala There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
How would that even be possible the way getters are defined? I don't really see an issue with the lines highlighted, and I'm not even sure why we'd need a getter that returns a single property. But going to the definition:
Sure, in this instance Now, this could maybe work by having a getter that takes the contractID as a function parameter, but this requires refactoring and adds complexity. I also think such getters that are simply accessors should be avoided, as they increase code line count and complexity for no benefit. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thinking about it, another solution along the lines of what you suggested could be something like: const getters = sbp('chelonia/contract/getters', 'gi.contracts/chatroom', state)
getters.currentChatRoomState But honestly, for simple accessors such as these I don't see the benefit of adding this complexity. This also adds two other factors:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @corrideat would it be possible to instantiate dynamically a new batch of getters where
Getters are very important, as they solve a lot of DRY related problems. By using getters we can ensure that the way that data is accessed everywhere will have consistent return values (e.g. because it uses tricks like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yeah, that's a great idea! I'd make one slight modification: instead of passing in const getters = sbp('chelonia/contract/getters', contractID) (EDIT: I don't think you need to pass in the name, that can be retrieved using Then yeah, at least we could reuse the contract getters that are already defined in the contracts! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Update based on today's conversation: we could use getters synchronously by importing the contract getters from this actions file. However, it'd have the downside of being 'statically hardcoded' based on whatever version is imported at build time and will increase bundle size. Doing it via Chelonia is potentially possible, but it'd need to be async in case the contract isn't cached and need to be loading and also maybe because of sandboxing. |
||
}).catch(e => { | ||
console.error('[action/chatroom.js] Error on messageReceivePostEffect', e) | ||
}) | ||
|
@@ -212,8 +213,7 @@ export default (sbp('sbp/selectors/register', { | |
} | ||
}, | ||
'gi.actions/chatroom/shareNewKeys': (contractID: string, newKeys) => { | ||
const rootState = sbp('state/vuex/state') | ||
const state = rootState[contractID] | ||
const state = sbp('chelonia/contract/state', contractID) | ||
|
||
const originatingContractID = state.attributes.groupContractID ? state.attributes.groupContractID : contractID | ||
|
||
|
@@ -251,43 +251,58 @@ export default (sbp('sbp/selectors/register', { | |
...encryptedAction('gi.actions/chatroom/join', L('Failed to join chat channel.'), async (sendMessage, params, signingKeyId) => { | ||
const rootState = sbp('state/vuex/state') | ||
const userID = params.data.memberID || rootState.loggedIn.identityContractID | ||
const userIDs = Array.isArray(userID) | ||
? userID.map(x => x == null ? rootState.loggedIn.identityContractID : x) | ||
: [userID] | ||
|
||
// We need to read values from both the chatroom and the identity contracts' | ||
// state, so we call wait to run the rest of this function after all | ||
// operations in those contracts have completed | ||
await sbp('chelonia/contract/wait', [params.contractID, userID]) | ||
await sbp('chelonia/contract/retain', userIDs, { ephemeral: true }) | ||
try { | ||
await sbp('chelonia/contract/wait', params.contractID) | ||
|
||
if (!userID || !has(rootState.contracts, userID)) { | ||
throw new Error(`Unable to send gi.actions/chatroom/join on ${params.contractID} because user ID contract ${userID} is missing`) | ||
} | ||
userIDs.forEach(cID => { | ||
if (!cID || !has(rootState.contracts, cID) || !has(rootState, cID)) { | ||
throw new Error(`Unable to send gi.actions/chatroom/join on ${params.contractID} because user ID contract ${cID} is missing`) | ||
} | ||
}) | ||
|
||
const CEKid = params.encryptionKeyId || await sbp('chelonia/contract/currentKeyIdByName', params.contractID, 'cek') | ||
const CEKid = params.encryptionKeyId || await sbp('chelonia/contract/currentKeyIdByName', params.contractID, 'cek') | ||
|
||
const userCSKid = sbp('chelonia/contract/currentKeyIdByName', userID, 'csk') | ||
return await sbp('chelonia/out/atomic', { | ||
...params, | ||
contractName: 'gi.contracts/chatroom', | ||
data: [ | ||
const userCSKids = await Promise.all(userIDs.map(async (cID) => | ||
[cID, await sbp('chelonia/contract/currentKeyIdByName', cID, 'csk')] | ||
)) | ||
return await sbp('chelonia/out/atomic', { | ||
...params, | ||
contractName: 'gi.contracts/chatroom', | ||
data: [ | ||
// Add the user's CSK to the contract | ||
[ | ||
'chelonia/out/keyAdd', { | ||
[ | ||
'chelonia/out/keyAdd', { | ||
// TODO: Find a way to have this wrapping be done by Chelonia directly | ||
data: [encryptedOutgoingData(params.contractID, CEKid, { | ||
foreignKey: `sp:${encodeURIComponent(userID)}?keyName=${encodeURIComponent('csk')}`, | ||
id: userCSKid, | ||
data: rootState[userID]._vm.authorizedKeys[userCSKid].data, | ||
permissions: [GIMessage.OP_ACTION_ENCRYPTED + '#inner'], | ||
allowedActions: '*', | ||
purpose: ['sig'], | ||
ringLevel: Number.MAX_SAFE_INTEGER, | ||
name: `${userID}/${userCSKid}` | ||
})] | ||
} | ||
data: userCSKids.map(([cID, cskID]: [string, string]) => encryptedOutgoingData(params.contractID, CEKid, { | ||
foreignKey: `sp:${encodeURIComponent(cID)}?keyName=${encodeURIComponent('csk')}`, | ||
id: cskID, | ||
data: rootState[cID]._vm.authorizedKeys[cskID].data, | ||
permissions: [GIMessage.OP_ACTION_ENCRYPTED + '#inner'], | ||
allowedActions: '*', | ||
purpose: ['sig'], | ||
ringLevel: Number.MAX_SAFE_INTEGER, | ||
name: `${cID}/${cskID}` | ||
})) | ||
} | ||
], | ||
...(Array.isArray(userID) | ||
? userID.map(cID => sendMessage({ ...params, data: { memberID: cID }, returnInvocation: true })) | ||
: [sendMessage({ ...params, returnInvocation: true })] | ||
) | ||
], | ||
sendMessage({ ...params, returnInvocation: true }) | ||
], | ||
signingKeyId | ||
}) | ||
signingKeyId | ||
}) | ||
} finally { | ||
await sbp('chelonia/contract/release', userIDs, { ephemeral: true }) | ||
} | ||
}), | ||
...encryptedAction('gi.actions/chatroom/rename', L('Failed to rename chat channel.')), | ||
...encryptedAction('gi.actions/chatroom/changeDescription', L('Failed to change chat channel description.')), | ||
|
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 it possible to keep the getter?
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 reason that the getter isn't being used here is that although I added a wrapper for root getters, these are Vuex modules, which work a bit differently due to state partitioning. At the time, the separate state keys being used by the module weren't there. Now they are, and it would be feasible to similarly have this work.
While I think that in this instance it'd be possible to use a getter, many getters are written to rely on
curentXXXId
, which is problematic in the SW.