-
Notifications
You must be signed in to change notification settings - Fork 307
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
refactor: redpacket history #2374
Conversation
zhouhanseng
commented
Feb 8, 2021
•
edited
Loading
edited
- deploy test-net contract
- update front-end
- review approved
- test pass
- deploy main-net contract
- test pass
- merge
After this PR merged, do i need to stop the older service?
CC @guanbinrui |
keep it running for a while just in case |
export async function getRedPacketsHistory(rpids: string[]) { | ||
const records: RedPacketRecord[] = [] | ||
for await (const record of RedPacketDatabase.iterate('red-packet')) { | ||
if (rpids.includes(record.payload.rpid)) records.push(RedPacketRecordOutDB(record)) |
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 have multiple records with the same rpid?
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.
No, each record has its only rpid.
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 why don't you get record by RedPacketDatabase.get
?
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 last question @zhouhanseng
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.
if you can query via .get
plz use it instead of iterate over the database. it's O1 (hash table)
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.
cc @zhouhanseng
packages/maskbook/src/plugins/RedPacket/hooks/useAllRedPackets.ts
Outdated
Show resolved
Hide resolved
export async function getRedPacketsHistory(rpids: string[]) { | ||
const records: RedPacketRecord[] = [] | ||
for await (const record of RedPacketDatabase.iterate('red-packet')) { | ||
if (rpids.includes(record.payload.rpid)) records.push(RedPacketRecordOutDB(record)) |
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 why don't you get record by RedPacketDatabase.get
?
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 on code style
oh wait |
|
const redPacketsFromDB = await database.getRedPacketsHistory(redPacketsFromChain.map((x) => x.rpid)) | ||
return redPacketsFromChain.reduce((acc, history) => { | ||
const record = redPacketsFromDB.find((y) => y.payload.rpid === history.rpid) | ||
if (history.chain_id === chainId && record) { |
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.
Check history.chain_id === chainId
at first and if it's true the finding logic can be ommited.
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.
It can't be ommited since we need password.
How's it? @zhouhanseng @septs @Jack-Works |
@Tedko work on this today. |
4cfae96
to
4706fdd
Compare
export interface RedPacketAvailability { | ||
token_address: string | ||
balance: string | ||
total: string | ||
claimed: string | ||
expired: boolean | ||
ifclaimed: boolean | ||
claimed_amount: string |
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 a breaking change?
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.
yes, already handled it:
Maskbook/packages/maskbook/src/plugins/RedPacket/hooks/useAvailabilityComputed.ts
Line 32 in 8accb67
const isClaimed = availability.claimed_amount !== '0' |
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 mean how do you handle the old name ifclaimed
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.
const isClaimed = availability.ifclaimed
=> const isClaimed = availability.claimed_amount !== '0'
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 IIRC the field "ifclaimed" is actually used in the payload. did it changed too?
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.
ifclaimed
does not exist in payload
d01507a
to
2f8c80e
Compare
@@ -14,6 +10,10 @@ | |||
"type": "string", | |||
"title": "rpid" | |||
}, | |||
"txid": { |
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.
It is not breaking change, since additionalProperties
is true
and "contract_version"
is removed from "required"
field, old red-packet payload is compatible.
const redPacketsFromDB = await database.getRedPacketsHistory(redPacketsFromChain.map((x) => x.txid)) | ||
return redPacketsFromChain.reduce((acc, history) => { | ||
const record = redPacketsFromDB.find((y) => y.id === history.txid) | ||
const historys = await subgraph.getRedPacketHistory(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.
simplify this logic
b3e51a5
to
5e00a2a
Compare
packages/maskbook/src/plugins/RedPacket/contracts/useRedPacketContract.ts
Show resolved
Hide resolved
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.
currently lgtm
merge with master. might have some type errors in it |
@Jack-Works Notice don't merge into master before mainnet contract deployed. |
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 on style
contract that applies improvements on gas
76f6955
to
9c42f1b
Compare
Test passed with ETH MainNet. |
state.error.code <= JSON_RPC_ErrorCode.SERVER_ERROR_RANGE_START && | ||
state.error.code >= JSON_RPC_ErrorCode.SERVER_ERROR_RANGE_END) |
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.
plz extract this code snippet as tool function