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

Fix default bridge byte fee to use TransactionByteFee of the bridged chain #174

Merged
merged 9 commits into from
Mar 4, 2024

Conversation

svyatonik
Copy link
Contributor

@svyatonik svyatonik commented Feb 1, 2024

This PR does two things:

  1. (tiny refactoring) adds TRANSACTION_BYTE_FEE constant to system-parachains-constants crate. We are using the same value (1 * MILLICENTS) across all chains, so it makes sense to move it there;
  2. changes default value of governance-controlled XcmBridgeHubRouterByteFee constant (which is used to cover the cost of message delivery within the remote consensus) to use TRANSACTION_BYTE_FEE of the bridged chain (also does 1:5/5:1 conversion). Since governance has not set value of this constant yet, we are changing the default value here, in the code.

The failing test is fixed by the #137, so it makes sense to merge this PR only after #137 Updated

@bkontur bkontur mentioned this pull request Feb 5, 2024
19 tasks
@svyatonik svyatonik force-pushed the sv-fix-bridge-byte-fee branch from 90bf015 to ca80d61 Compare February 15, 2024 10:46
@@ -248,7 +252,7 @@ impl pallet_balances::Config for Runtime {

parameter_types! {
/// Relay Chain `TransactionByteFee` / 10
pub const TransactionByteFee: Balance = MILLICENTS;
pub const TransactionByteFee: Balance = TRANSACTION_BYTE_FEE;
Copy link
Contributor

Choose a reason for hiding this comment

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

same

Copy link
Contributor

Choose a reason for hiding this comment

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

@kianenigma could you please re-check this commit: c5d2467 and if ok, then close this comment

Copy link
Contributor

Choose a reason for hiding this comment

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

got "ok" by element

@bkontur
Copy link
Contributor

bkontur commented Mar 1, 2024

/merge

@fellowship-merge-bot
Copy link
Contributor

Auto-Merge-Bot

User @bkontur is not the author of the PR and does not publicly belong to the org polkadot-fellows.

Only author or public org members can trigger the bot.

@svyatonik
Copy link
Contributor Author

/merge

@fellowship-merge-bot
Copy link
Contributor

There was a problem running the action.

❌😵❌

Please find more information in the logs.

@svyatonik
Copy link
Contributor Author

/merge

@fellowship-merge-bot
Copy link
Contributor

There was a problem running the action.

❌😵❌

Please find more information in the logs.

@svyatonik
Copy link
Contributor Author

@Bullrich You've already made some magic us with my previous fellowship PR here. But it looks like I'm doomed - now we get this "Pull request Pull request is in unstable status" error message here. Could you, please, check when you have a minute - whether it is my or some github/bot issue? Thank you!

@Bullrich
Copy link
Contributor

Bullrich commented Mar 1, 2024

@Bullrich You've already made some magic us with my previous fellowship PR here. But it looks like I'm doomed - now we get this "Pull request Pull request is in unstable status" error message here. Could you, please, check when you have a minute - whether it is my or some github/bot issue? Thank you!

I'm looking into it.

In theory it should merge it if it's in clean status

From auto-merge-bot

  async enableAutoMerge(): Promise<void> {
    try {
      await this.gql<{
        enablePullRequestAutoMerge: { clientMutationId: unknown };
      }>(ENABLE_AUTO_MERGE, {
        prId: this.nodeId,
        mergeMethod: this.mergeMethod,
      });
      this.logger.info("Succesfully enabled auto-merge");
    } catch (error) {
      this.logger.warn(error as Error);
      if (
        error instanceof Error &&
        error.message.includes("Pull request is in clean status")
      ) {
        this.logger.warn(
          "Pull Request is ready to merge. Running merge command instead",
        );
        await this.gql<{
          mergePullRequest: { clientMutationId: unknown };
        }>(MERGE_PULL_REQUEST, {
          prId: this.nodeId,
          mergeMethod: this.mergeMethod,
        });
        this.logger.info("Succesfully merged PR");
      } else {
        throw error;
      }
    }
  }

but I'm not sure what unstable means in this case.

@bkchr, does it allow you to merge? Are all the approvals from members of polkadot-fellows? My only idea is that maybe you need an approval review from a member?

I have also been looking online but I can't find anything about what unstable means. The closest thing I found is in an action that inspired this one:

@Bullrich
Copy link
Contributor

Bullrich commented Mar 1, 2024

@svyatonik looking a bit more in depth, could this maybe be the problem?

image

I'm just speculating, but this could define it as unstable because not all the status checks passed. In theory, #192 would force all the checks to be green and, if this is the problem (again, I'm speculating) it could be fixed.

@svyatonik
Copy link
Contributor Author

svyatonik commented Mar 1, 2024

@svyatonik looking a bit more in depth, could this maybe be the problem?

image I'm just speculating, but this could define it as unstable because not all the status checks passed. In theory, #192 would force all the checks to be green and, if this is the problem (again, I'm speculating) it could be fixed.

I've been thinking the same, but then I looked at recently merged PRs and they had the same issue: e.g. #178 (manual merge) and #204. If you have no ideas - let's try later maybe. Maybe that's really some GH issue (though status shows it is ok)

@acatangiu
Copy link
Contributor

/merge

@fellowship-merge-bot
Copy link
Contributor

There was a problem running the action.

❌😵❌

Please find more information in the logs.

@svyatonik
Copy link
Contributor Author

/merge

@fellowship-merge-bot
Copy link
Contributor

There was a problem running the action.

❌😵❌

Please find more information in the logs.

@svyatonik
Copy link
Contributor Author

I could recreate a PR if approvers will help me with getting secondary approves in a new PR. Hopefully it'll help. Otherwise, IIUC, anyone with write permissions could manually merge this PR.

@bkchr bkchr merged commit 55e97c5 into polkadot-fellows:main Mar 4, 2024
31 of 32 checks passed
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.

7 participants