Skip to content

Unprofitable message delivery tx metric#1536

Merged
svyatonik merged 8 commits into
masterfrom
unprofitable-transaction-metric
Aug 5, 2022
Merged

Unprofitable message delivery tx metric#1536
svyatonik merged 8 commits into
masterfrom
unprofitable-transaction-metric

Conversation

@svyatonik
Copy link
Copy Markdown
Contributor

related to #1522

WIP

@svyatonik svyatonik marked this pull request as ready for review August 4, 2022 13:06
@svyatonik svyatonik marked this pull request as draft August 4, 2022 13:10
@svyatonik svyatonik marked this pull request as ready for review August 5, 2022 06:44
@svyatonik svyatonik enabled auto-merge (squash) August 5, 2022 06:44
@svyatonik svyatonik merged commit 9bae4a9 into master Aug 5, 2022
@svyatonik svyatonik deleted the unprofitable-transaction-metric branch August 5, 2022 07:00
Comment on lines +43 to +45
// we don't care about costs and rewards, but we want to report unprofitable transactions
// => let rational strategy fill required fields
let _ = RationalStrategy.decide(reference).await;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Could we move the entire logic that computes the costs and rewards to a common method (could be defined on the RelayStrategy trait) ? It seems strange to use RationalStrategy.decide() inside AltruisticStrategy.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'd leave it outside trait, but feel free to implement what you think is better

true
}

async fn final_decision<
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

2 questions:

  1. Do we need this method to be async ?
  2. This method doesn't seem to be used for taking a decision. Could we rename it to something like update_metrics ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

  1. If it works without async, let's remove it - it has probably leftover of some early experiments
  2. I mean something like "notify strategy about final decision", but feel free to rename

@serban300 serban300 mentioned this pull request Aug 18, 2022
serban300 added a commit that referenced this pull request Aug 18, 2022
* Make RelayStrategy::final_decision() sync

Signed-off-by: Serban Iorga <serban@parity.io>

* Move logic from RelayStrategy to RelayReference

Signed-off-by: Serban Iorga <serban@parity.io>

* Rename RelayStrategy::final_decision()

Signed-off-by: Serban Iorga <serban@parity.io>
@serban300
Copy link
Copy Markdown
Collaborator

The comments were addressed in #1549 . Marking this PR as reviewed.

serban300 pushed a commit to serban300/parity-bridges-common that referenced this pull request Mar 27, 2024
* unprofitable message delivery tx metric

* proper impl

* send Rialto -> Millau messages using XCM pallet

* use altruistic relays in Rialto <> Millau bridge

* add unprofitable transactions dashboard

* fix + logging

* fix test
serban300 added a commit to serban300/parity-bridges-common that referenced this pull request Mar 27, 2024
* Make RelayStrategy::final_decision() sync

Signed-off-by: Serban Iorga <serban@parity.io>

* Move logic from RelayStrategy to RelayReference

Signed-off-by: Serban Iorga <serban@parity.io>

* Rename RelayStrategy::final_decision()

Signed-off-by: Serban Iorga <serban@parity.io>
serban300 pushed a commit to serban300/parity-bridges-common that referenced this pull request Apr 8, 2024
* unprofitable message delivery tx metric

* proper impl

* send Rialto -> Millau messages using XCM pallet

* use altruistic relays in Rialto <> Millau bridge

* add unprofitable transactions dashboard

* fix + logging

* fix test
serban300 added a commit to serban300/parity-bridges-common that referenced this pull request Apr 8, 2024
* Make RelayStrategy::final_decision() sync

Signed-off-by: Serban Iorga <serban@parity.io>

* Move logic from RelayStrategy to RelayReference

Signed-off-by: Serban Iorga <serban@parity.io>

* Rename RelayStrategy::final_decision()

Signed-off-by: Serban Iorga <serban@parity.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants