This repository has been archived by the owner on Nov 15, 2023. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The Ambassador Program #2002
base: master
Are you sure you want to change the base?
The Ambassador Program #2002
Changes from 13 commits
f885e71
8e76281
0541b4b
5cf1abf
07075dd
80df39c
8c288e3
5bd566d
5356d11
8112e48
0ffbaaf
b81ba28
9b89c1d
bbe116c
6104edb
f0c3a60
514fce9
1323be3
f200aef
1b867de
174a9a8
9017a8b
d602443
07c7d86
fe57ff2
5f111a2
2ef13d0
48363a7
a9926fb
5313c68
cb79c45
f867832
35e0417
d515363
c238fb2
9ab6aa4
67902de
54bd222
d5118b0
15ad2f5
9a562c5
08153e7
b7d8da7
273e9af
9ae1f68
fd88108
56baa6a
8efb683
f9c1c06
40bf76e
69036ba
421d3cf
1d625c0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ive seen few implementations of the CID in or repos (at least two).
Here I keep it simple instead introducing new. I believe we need to agree first on one type for it, and keep it consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Storing CID as a bounded vec sounds good to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have a type for cid, maybe we should push it down to a more common place:
https://github.com/paritytech/substrate/blob/8cfe326e4e33c5077fc67f197d6a13dd871881c7/frame/alliance/src/types.rs#L69
We shouldn't have multiple different types for cids.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The alternative is we could call this type an OpaqueCid maybe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renaming to OpaqueCid
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if this setup works well for the Ambassador program, we might translate the Alliance into the similar setup, this is why the pallet generic over instance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curiosity killed the cat, but everyone is going to want to know why an announcement has been removed. Would be cool to have an enum of Outdated, Retracted, Other as to the announcement removal reason.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
worst case isn't it O(log(MaxAnnoucementsCount)) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
totally, binary search is ~logN complexity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not, the
Weight
of this call isO(1)
.You are talking about its complexity