Skip to content
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

Implement Push Publisher #27

Open
wants to merge 11 commits into
base: develop
Choose a base branch
from
Open

Implement Push Publisher #27

wants to merge 11 commits into from

Conversation

GoodluckH
Copy link
Contributor

@GoodluckH GoodluckH commented Jul 15, 2022


tsconfig.json Outdated Show resolved Hide resolved
@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
Contributor Author

/rebase-autosquash

Copy link
Contributor

@thehobbit85 thehobbit85 left a comment

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
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

src/publishers/push.ts Outdated Show resolved Hide resolved
@swansontec swansontec changed the base branch from master to develop July 19, 2022 17:29
@swansontec
Copy link
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 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
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.

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