Conversation
…to feat/vest-airdrop-rewards
vrom911
reviewed
May 21, 2025
Contributor
vrom911
left a comment
vrom911
left a comment
There was a problem hiding this comment.
Nice work! Left a few comments
| AirdropId, | ||
| <<T as Config>::Currency as Inspect<T::AccountId>>::Balance, | ||
| ValueQuery, | ||
| AirdropMetadata<BlockNumberFor<T>, BalanceOf<T>, T::AccountId>, |
Contributor
There was a problem hiding this comment.
I see now that we have a single map. In the previous implementation, we had separate maps to separate access to the different parts in different functions. I wonder if this is more optimal. Or is it not a problem to work with bigger maps?
Collaborator
There was a problem hiding this comment.
I think we should talk about the costs of storing data in unlimited maps in general. We've tried to limit it as much as possible in the past, but of course sometimes it's not possible.
Either way, this change seems sensible.
Contributor
Author
There was a problem hiding this comment.
I wonder if this is more optimal. Or is it not a problem to work with bigger maps?
it depends obv, we could keep your way of declaring separate maps, but reasons I opted for this:
- struct itself isn't that large (32 + 32 + 16 + 4 + 4 = 88 bytes at most), so there's not much cost that incurs when let's say we read/write the whole metadata but only use/change one field
- when I need to add info about vesting period and delay, I need to create two more maps with the same structure and it makes the code bloated and increases the cognitive load. Every time we change sth, we have to think if it touches the other map, etc.
- most probably the most used extrinsic will be
claim. And in there we access all the maps, which makes it multiple write/reads with separate maps. and with this approach it's single read/write.
I think we should talk about the costs of storing data in unlimited maps in general.
unlimited maps are not the problem per se (f.e System::Account is unlimited but it's mitigated by ED). but we def need to make sure to charge deposit fee for airdrop creation to prevent possible spam
…to feat/vest-airdrop-rewards
Collaborator
|
Is there a unit test for the case where the funding account has insufficient funds? Didn't see one but I would like to be sure about what happens in this fail case. ie... Alice funds Airdrop with 100, but only has 10 balance. |
Collaborator
|
Not sure about test coverage, rest looks good |
n13
approved these changes
May 23, 2025
Contributor
Author
added insufficient balance check to |
dastansam
added a commit
that referenced
this pull request
Jun 4, 2025
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * remove caching * Only allow airdrop creators to delete it * Allow creators to claim remain balance when delete * Clean up docs * Remove unsignedValidation, add tests * Remove unnecessary error * Generate weights from the nenchmarking * Use the existing weights template * Use unsigned acc for claims --------- Co-authored-by: illuzen <github.handheld844@passmail.net> feat: Tech Collective for Runtime Upgrades (#12) * feat: Membership configuration * feat: Founders in genesis config * feat: First community test * feat: TxExtension - only members can vote for track 0 * feat: Track 0 for members only without token support validation * feat: Proposals for track 0 limited to tech collective members * fix: TxExt rename in node * feat: Genesis presets - tech collective members * chore: Tracks params update * feat: Separate referenda with submit limited * feat: Separate tracks - ready for test fix * feat: Membership removed * feat: First test for TechCollective * feat: Tests for new collective ready * feat: All tests fixed and cleaned * clean: TxExtension * fix: Review comments + formatting * chore: MaxRankOfClass - additional comments Add windows build to release process (#17) windows takes 30 minutes to release... remove special case for 0.0.0 - just use it (#18) Bump poseidon-resonance to 0.4.0 (#20) * Bump poseidon-resonance to 0.4.0 * Update to 0.5.0 chore: Chain-spec JSON - refreshed (#24) feat: vest airdrop rewards (#22) * Replace vesting pallet * Vest airdrops * Update runtime * Resolve comments * Add insufficient balance check move rusty-crystals to github (#26) * move rusty-crystals to github * poseidon-resonance 0.5.0 CLI Updates (#27) * move rusty-crystals to github * CLI updates * moved test data to file * remove redundant comments * poseidon-resonance 0.5.0 * merge from main * Update readme for new CLI commands Make benchmarks compile (#29) feat: explicit reverser for reversible accounts (#8) * Add explicit reverser for reversible transfers * Make reverser get reserved funds, update benchmarks * Add more tests updated storage hasher for transfer proofs (#25) * updated storage hasher for transfer proofs * minor cleanup * fixed some tests * tests compile now * Pass AccountId for storage hashing (#30) * tests fixed --------- Co-authored-by: illuzen <github.handheld844@passmail.net> Co-authored-by: Dastan <88332432+dastansam@users.noreply.github.com> cargo fmt (#31) applied cargo fmt to codebase Update ci.yml (#32) add cargo fmt check don't cache on linux (#33) prevents out of disk space errors Fix compile errors Run fmt Mostly passing tests
dastansam
added a commit
that referenced
this pull request
Jun 4, 2025
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * remove caching * Only allow airdrop creators to delete it * Allow creators to claim remain balance when delete * Clean up docs * Remove unsignedValidation, add tests * Remove unnecessary error * Generate weights from the nenchmarking * Use the existing weights template * Use unsigned acc for claims --------- Co-authored-by: illuzen <github.handheld844@passmail.net> feat: Tech Collective for Runtime Upgrades (#12) * feat: Membership configuration * feat: Founders in genesis config * feat: First community test * feat: TxExtension - only members can vote for track 0 * feat: Track 0 for members only without token support validation * feat: Proposals for track 0 limited to tech collective members * fix: TxExt rename in node * feat: Genesis presets - tech collective members * chore: Tracks params update * feat: Separate referenda with submit limited * feat: Separate tracks - ready for test fix * feat: Membership removed * feat: First test for TechCollective * feat: Tests for new collective ready * feat: All tests fixed and cleaned * clean: TxExtension * fix: Review comments + formatting * chore: MaxRankOfClass - additional comments Add windows build to release process (#17) windows takes 30 minutes to release... remove special case for 0.0.0 - just use it (#18) Bump poseidon-resonance to 0.4.0 (#20) * Bump poseidon-resonance to 0.4.0 * Update to 0.5.0 chore: Chain-spec JSON - refreshed (#24) feat: vest airdrop rewards (#22) * Replace vesting pallet * Vest airdrops * Update runtime * Resolve comments * Add insufficient balance check move rusty-crystals to github (#26) * move rusty-crystals to github * poseidon-resonance 0.5.0 CLI Updates (#27) * move rusty-crystals to github * CLI updates * moved test data to file * remove redundant comments * poseidon-resonance 0.5.0 * merge from main * Update readme for new CLI commands Make benchmarks compile (#29) feat: explicit reverser for reversible accounts (#8) * Add explicit reverser for reversible transfers * Make reverser get reserved funds, update benchmarks * Add more tests updated storage hasher for transfer proofs (#25) * updated storage hasher for transfer proofs * minor cleanup * fixed some tests * tests compile now * Pass AccountId for storage hashing (#30) * tests fixed --------- Co-authored-by: illuzen <github.handheld844@passmail.net> Co-authored-by: Dastan <88332432+dastansam@users.noreply.github.com> cargo fmt (#31) applied cargo fmt to codebase Update ci.yml (#32) add cargo fmt check don't cache on linux (#33) prevents out of disk space errors Fix compile errors Run fmt Mostly passing tests
dastansam
pushed a commit
that referenced
this pull request
Jun 4, 2025
chore: Only more logs - no changes chore: Less logs fix: O3 solution fix: Finalization triggered often make difficulty actually difficulty removed unused code, added chain_height to prometheus add difficulty back improved logging, clamped difficulty adjustments, raised max_distance, fixed pct calculation, changed block history to 10 and adjustment period to 1 fix: Spec JSON + QUAN token fixed difficulty reporting to prometheuss changed license to MIT fixed 3 tests cleaned up logs Update external-miner/src/lib.rs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> fixed external miner chore: ChainSpec refresh + rename to Quantus chore: Resonance as our testnet name ci: github workflow build, test rename node chore: CLI: Resonance Key -> Quantus Key (#13) Use clippy (#11) * Apply clippy autofix * Fix comparison_chain & doc_lazy_continuation * Fix more warnings Update README.md Update README.md Automated release workflow (#16) * added create release workflow * shell script added * rename * remove caching Merkle airdrop pallet (#10) * Implement initial structure of merkle-airdrop * Improve structure, add all template modules * Implement create_airdrop and fix tests and mock * Implement verify merkle proof, add tests * Use binary_merkle_tree instead of custom function * Implement fund function with tests * implement claim function * Clean up * Use poseidon hasher in verify fn * Try to create merkle root for testing * Add doc for testing * Remove rust-toolchain.toml from git tracking * Remove js script * Make claim recipient be anyone * Revert the signer-receiver change, add test fns * changed hash fn to blake2 * Apply fmt * Fix tests * Add validate_unsigned * Fix link to merkle-airdrop-cli * Address review comments * Remove MaxAirdrops * Add UnsignedClaimPriority in the config * Add delete airdrop functionality * Replace deprecated Currency * Only allow airdrop creators to delete it * Allow creators to claim remain balance when delete * Clean up docs * Remove unsignedValidation, add tests * Remove unnecessary error * Generate weights from the nenchmarking * Use the existing weights template * Use unsigned acc for claims --------- Co-authored-by: illuzen <github.handheld844@passmail.net> feat: Tech Collective for Runtime Upgrades (#12) * feat: Membership configuration * feat: Founders in genesis config * feat: First community test * feat: TxExtension - only members can vote for track 0 * feat: Track 0 for members only without token support validation * feat: Proposals for track 0 limited to tech collective members * fix: TxExt rename in node * feat: Genesis presets - tech collective members * chore: Tracks params update * feat: Separate referenda with submit limited * feat: Separate tracks - ready for test fix * feat: Membership removed * feat: First test for TechCollective * feat: Tests for new collective ready * feat: All tests fixed and cleaned * clean: TxExtension * fix: Review comments + formatting * chore: MaxRankOfClass - additional comments Add windows build to release process (#17) windows takes 30 minutes to release... remove special case for 0.0.0 - just use it (#18) Bump poseidon-resonance to 0.4.0 (#20) * Bump poseidon-resonance to 0.4.0 * Update to 0.5.0 chore: Chain-spec JSON - refreshed (#24) feat: vest airdrop rewards (#22) * Replace vesting pallet * Vest airdrops * Update runtime * Resolve comments * Add insufficient balance check move rusty-crystals to github (#26) * move rusty-crystals to github * poseidon-resonance 0.5.0 CLI Updates (#27) * move rusty-crystals to github * CLI updates * moved test data to file * remove redundant comments * poseidon-resonance 0.5.0 * merge from main * Update readme for new CLI commands Replace scheduler in reversible transfers Make benchmarks compile (#29) feat: explicit reverser for reversible accounts (#8) * Add explicit reverser for reversible transfers * Make reverser get reserved funds, update benchmarks * Add more tests updated storage hasher for transfer proofs (#25) * updated storage hasher for transfer proofs * minor cleanup * fixed some tests * tests compile now * Pass AccountId for storage hashing (#30) * tests fixed --------- Co-authored-by: illuzen <github.handheld844@passmail.net> Co-authored-by: Dastan <88332432+dastansam@users.noreply.github.com> cargo fmt (#31) applied cargo fmt to codebase Update ci.yml (#32) add cargo fmt check don't cache on linux (#33) prevents out of disk space errors Compiling node Make legacy tests work Fix compile errors Run fmt Mostly passing tests
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
vestingfork with Substrate officialNote: benchmarks are not updated yet, will be a part of a bigger general benchmarks/weight cleanup PR later