Skip to content

Ephesus: add optional payment to member remark#4517

Merged
Lezek123 merged 17 commits intoJoystream:ephesusfrom
zeeshanakram3:ephesus_addPaymentToMemberRemark
Jan 25, 2023
Merged

Ephesus: add optional payment to member remark#4517
Lezek123 merged 17 commits intoJoystream:ephesusfrom
zeeshanakram3:ephesus_addPaymentToMemberRemark

Conversation

@zeeshanakram3
Copy link
Copy Markdown
Contributor

@zeeshanakram3 zeeshanakram3 commented Jan 10, 2023 •
edited by sync-by-unito bot
Loading

Addresses #4507. This PR addresses all the relevant runtime, QN and integration tests work.

depends on Joystream/hydra#513

message MakeChannelPayment {
    // Reason why payment is being made
    optional string rationale = 1;

    // Context of payment, if it is being made for a specific video, channel, or from an app/gateway
    oneof payment_context {
        // payment as a tip associated with a specific video
        uint64 video_id = 2;
        // payment as a channel wide tip
        uint64 channel_id = 3;
        // payment from member representing a gateway
        uint32 app_id = 4;
    }
}
  • Adds the following QN entities:
type PaymentContextVideo @variant {
  "Video for which the payment was made"
  video: Video!
}

type PaymentContextChannel @variant {
  "Channel for which the payment was made"
  channel: Channel!
}

type PaymentContextApp @variant {
  "Gateway/App that made the payment to specific channel"
  app: App!
}

"Various Channel Payment Contexts"
union PaymentContext = PaymentContextVideo | PaymentContextChannel | PaymentContextApp

"Direct channel payment by any member by-passing the council payouts."
type ChannelPaymentMadeEvent @entity {
  ### GENERIC DATA ###
  # ...
  
  ### SPECIFIC DATA ###

  "Actor that made the payment"
  payer: Membership!

  "Amount of the payment"
  amount: BigInt!

  "Payment and payee context"
  paymentContext: PaymentContext

  "Channel that received the payment (if any)"
  payeeChannel: Channel

  "Reason of the payment"
  rationale: String
}
  • Although payment can be sent with any arbitrary message type, however, right now, mappings will only process the payment if the metadata message type is MakeChannelPayment. Maybe, we can also support sending payment with other existing metaprotocol messages e.g., ReactComment, with the intention of tipping the comment creator. This is one example, etc.

┆Issue is synchronized with this Asana task by Unito

@vercel
Copy link
Copy Markdown

vercel bot commented Jan 10, 2023 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated
pioneer-testnet ⬜️ Ignored (Inspect) Jan 25, 2023 at 3:21PM (UTC)

@zeeshanakram3 zeeshanakram3 mentioned this pull request Jan 10, 2023
Merged
dobertRowneySr
dobertRowneySr suggested changes Jan 12, 2023
View reviewed changes
Copy link
Copy Markdown
Collaborator

@dobertRowneySr dobertRowneySr left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Great job, I would add the following recommendations:

  • The extrinsic member_remark should be benchmarked again
  • I think you should add an edge case, where the extrinsic succeeds but the channel reward account is not found by the QN. Since you have added just the happy path case

tests/network-tests/src/fixtures/content/channelPayouts/DirectChannelPaymentFixture.ts
export type ChannelPaymentParams = {
asMember: MemberId
msg: IMakeChannelPayment
payment: [string, BN]
Copy link
Copy Markdown
Collaborator

@dobertRowneySr dobertRowneySr Jan 12, 2023

Choose a reason for hiding this comment

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

Suggested change
payment: [string, BN]
payment: [string, BN] // [address, amount]

@dobertRowneySr dobertRowneySr requested review from dobertRowneySr and removed request for dobertRowneySr January 12, 2023 07:51
@dmtrjsg dmtrjsg mentioned this pull request Jan 17, 2023
Closed
1 task
@bedeho
Copy link
Copy Markdown
Member

bedeho commented Jan 17, 2023

Since this is going into ephesus, where #4307 is not in scope, let us drop uint32 app_id = 4;, make sure to update #4518 also.

@dmtrjsg dmtrjsg mentioned this pull request Jan 17, 2023
Closed
3 tasks
dobertRowneySr
dobertRowneySr suggested changes Jan 19, 2023
View reviewed changes
Copy link
Copy Markdown
Collaborator

@dobertRowneySr dobertRowneySr left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The requests are addressed 👍 however there are some yarn lint errors, probably you need to re-build the metadata-protobuf package yarn workspace @joystream/metadata-protobuf build should update them (this is needed because appId was removed)

@Lezek123 Lezek123 mentioned this pull request Jan 19, 2023
Closed
32 tasks
@zeeshanakram3 zeeshanakram3 mentioned this pull request Jan 22, 2023
Merged
Lezek123
Lezek123 suggested changes Jan 23, 2023
View reviewed changes
Lezek123
Lezek123 reviewed Jan 25, 2023
View reviewed changes
query-node/mappings/src/content/channel.ts Outdated
@@ -600,9 +590,13 @@ export async function processChannelPaymentFromMember(
`queried video based on payee address, EXPECTED: ${channel.id}, ACTUAL: ${video.channel.id}`
Copy link
Copy Markdown
Contributor

@Lezek123 Lezek123 Jan 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The message here should say that the video's channelId does not match with the channel that was queried based on reward (or payee) account.

Alternatively you can provide channelId as part of the where condition when querying for the video, so that you only search within the correct channel to begin with.

Edit: I now also noticed that you don't return here, the way you do if the video is not found. I think it's better to treat both those cases the same way (ie. if the video does not belong to the channel, the context is invalid and should be set to null)

@Lezek123 Lezek123 self-requested a review January 25, 2023 07:55
Lezek123
Lezek123 suggested changes Jan 25, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@Lezek123 Lezek123 left a comment

Choose a reason for hiding this comment

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

See previous review (I accidentally submitted it by adding a comment)

@zeeshanakram3 zeeshanakram3 mentioned this pull request Jan 25, 2023
Open
@Lezek123 Lezek123 self-requested a review January 25, 2023 16:23
Lezek123
Lezek123 approved these changes Jan 25, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@Lezek123 Lezek123 left a comment

Choose a reason for hiding this comment

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

LGTM

@Lezek123 Lezek123 merged commit 915ef9c into Joystream:ephesus Jan 25, 2023
@dmtrjsg dmtrjsg mentioned this pull request Jan 30, 2023
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@dobertRowneySr dobertRowneySr dobertRowneySr requested changes

@Lezek123 Lezek123 Lezek123 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@Lezek123 Lezek123

Labels

network-integration-test End-to-end full network integration test query-node runtime

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@zeeshanakram3 @bedeho @Lezek123 @dobertRowneySr