Skip to content

Implement Push Publisher#27

Open
GoodluckH wants to merge 11 commits into
developfrom
xipu/push-publisher
Open

Implement Push Publisher#27
GoodluckH wants to merge 11 commits into
developfrom
xipu/push-publisher

Conversation

@GoodluckH

@GoodluckH GoodluckH commented Jul 15, 2022

Copy link
Copy Markdown
Contributor

@GoodluckH GoodluckH requested a review from thehobbit85 July 15, 2022 21:49
Comment thread tsconfig.json Outdated
@github-actions github-actions Bot force-pushed the xipu/push-publisher branch from 6bb1692 to 5715a0c Compare July 15, 2022 23:19
@GoodluckH GoodluckH force-pushed the xipu/push-publisher branch 2 times, most recently from 9f6ec41 to 6bb1692 Compare July 15, 2022 23:33
@GoodluckH

Copy link
Copy Markdown
Contributor Author

/rebase-autosquash

@github-actions github-actions Bot force-pushed the xipu/push-publisher branch from 6bb1692 to 97e8c80 Compare July 15, 2022 23:34
@GoodluckH GoodluckH requested a review from thehobbit85 July 15, 2022 23:45

@thehobbit85 thehobbit85 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we agreed that the ActionEffect type is going to be:

{
  type,
  completed,
  program
}

Instead of spreading all the props no?

@GoodluckH

Copy link
Copy Markdown
Contributor Author

I thought we agreed that the ActionEffect type is going to be:

{
  type,
  completed,
  program
}

Instead of spreading all the props no?

During the call, I thought we are going to use Sam's front-end implementation of ActionEffects? He also sent the code snippet for the type here: https://edgesecure.slack.com/archives/C03PG9LVDM3/p1657832756200729

@GoodluckH GoodluckH requested a review from thehobbit85 July 18, 2022 17:45
Comment thread src/publishers/push.ts Outdated
@GoodluckH GoodluckH requested a review from thehobbit85 July 18, 2022 20:57
@swansontec swansontec changed the base branch from master to develop July 19, 2022 17:29
@swansontec

Copy link
Copy Markdown
Contributor

I have switched this branch from master to develop. So, if it gets merged, it will end up there and not on master.

Comment thread src/types/task/ActionEffect.ts Outdated
Comment on lines +20 to +45
export interface SeqActionEffect extends GeneralActionEffect {
type: 'seq'
opIndex: number
childEffect: ActionEffect
}

export interface ParActionEffect extends GeneralActionEffect {
type: 'par'
childEffects: ActionEffect[]
}

export interface BalanceActionEffect extends GeneralActionEffect {
type: 'balance'
address: string
aboveAmount?: string
belowAmount?: string
walletId: string
tokenId?: string
}

export interface TxConfsActionEffect extends GeneralActionEffect {
type: 'tx-confs'
txId: string
walletId: string
confirmations: number
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are client-side types, not server types. I talked to Sam about this, and we agreed that the client needs to "bake" these down a bit more before sending them to the server.

For instance, walletId is useless to the server, since the server cannot access customer wallets. Instead, the client needs to extract a single network address for the server to check. Likewise, par and seq are something the client worries about as it executes the program. Once the client figures out what the next steps are, it uploads them to the server, so the server doesn't need to know how they are linked.

Nevertheless, I think we can go ahead and merge this now and fix it later - I don't want to slow down the project, and something is better than nothing.

@GoodluckH GoodluckH force-pushed the xipu/push-publisher branch from 0822833 to 5d964f5 Compare July 19, 2022 22:12
@GoodluckH GoodluckH requested a review from swansontec July 19, 2022 22:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants