Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Upward Message Passing implementation #1885

Merged
14 commits merged into from
Nov 2, 2020
Merged

Upward Message Passing implementation #1885

14 commits merged into from
Nov 2, 2020

Conversation

pepyakin
Copy link
Contributor

Closes #1677
The UMP part of and Closes #1702
The UMP part of #1869
Split from #1679

@pepyakin pepyakin added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Oct 29, 2020
Copy link
Contributor

@coriolinus coriolinus left a comment

Choose a reason for hiding this comment

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

Looks good to me! I haven't done a detailed review of ump.rs, but at a quick read-through I didn't see any problems there.

runtime/parachains/src/configuration.rs Outdated Show resolved Hide resolved
runtime/parachains/src/router/ump.rs Show resolved Hide resolved
Co-authored-by: Bernhard Schuster <bernhard@ahoi.io>
//
// let's select 0 as the starting index as a safe bet.
debug_assert!(false);
0
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a case this could be called with zero dispatchables and this hence being oob?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nah, cur_idx doesn't have to point on an existing element, we always access the underlying vector with .get

Copy link
Contributor

Choose a reason for hiding this comment

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

Just saying, using an Option<usize> might not be entirely silly in the long run.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Try to imagine how would it look like in the current version. We would have to add a couple of maps (or god forbid expects). I am not sure that it worth it presently. That said, I don't disagree that it might be more appropriate in the future

}
}

#[cfg(test)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably a good idea to split into two files here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

while I usually prefer doing splitting, I don't think this is a good case for that. First, we would have either ump_tests.rs (since the /router hosts a bunch of different modules already) or another level of hierarchy like router/ump.rs and router/ump/tests.rs, and both options seem awkward. Secondly, the tests are still manageable.

Copy link
Contributor

@drahnr drahnr left a comment

Choose a reason for hiding this comment

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

I very like the precise documentation in router and the impl gudie 👍 code LGTM as well (no deep dive though).

@pepyakin
Copy link
Contributor Author

pepyakin commented Nov 2, 2020

bot merge

@ghost
Copy link

ghost commented Nov 2, 2020

Trying merge.

@ghost ghost merged commit 6447cb5 into master Nov 2, 2020
@ghost ghost deleted the ser-ump branch November 2, 2020 14:20
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implementer's Guide: Dekindification / XCM Implement UMP
4 participants