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

Updates needed for substrate #4820 (composite accounts) #834

Closed
wants to merge 50 commits into from

Conversation

gavofyork
Copy link
Member

No description provided.

@jacogr
Copy link
Contributor

jacogr commented Feb 10, 2020

Tested this against an API with the type Address = AccountId and transfers as well as remarks works as expected. Checked the transactions -

  • "from" address passed as an AccountId (32-bytes )works as expected (in unchecked)
  • using AccountId as lookup source (e.g. transfer "to") works as expected
  • data shows up under system.account (balances)

So functionally it looks correct from my early testing. Will run through more.

Edit/Update:

  • as expected, on new accounts a new index is not created (and no event)
  • merged support for running polkadot --dev in the JS API 1 2-beta.5 as well as current deployed apps UI (this also applies to Kusama 1046+, this can be adjusted if it only goes much later)

Overall, lgtm, does what it says on the tin.

With paritytech/substrate#4767 a `GossipEngine`
does not spawn its own tasks, but relies on its owner to poll it. This
patch removes the unneeded executor argument and spawns the gossip
engine as an orphaned task onto the executor instead.

// If this fails, destination account already has a vesting schedule
// applied to it, and this claim should not be processed.
ensure!(
Copy link
Member

Choose a reason for hiding this comment

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

When you removed this ensure check, I also believe you now made it possible for someone with a vesting schedule to drop the pot to zero by calling this function over and over.

Copy link
Member

Choose a reason for hiding this comment

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

I have updated the logic and added a test which would have failed if this logic didn't exist.

15fe7cc

Copy link
Member

@shawntabrizi shawntabrizi 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. Would be good to get second review of the fix I pushed into the claims

Copy link
Contributor

@jacogr jacogr left a comment

Choose a reason for hiding this comment

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

Been testing it all the way through, so functionality-wise lgtm.

@NikVolf
Copy link
Contributor

NikVolf commented Feb 19, 2020

I've included changes in #853

@andresilva
Copy link
Contributor

Superseded by #853.

@andresilva andresilva closed this Feb 19, 2020
@shawntabrizi shawntabrizi deleted the gav-composite-account branch February 19, 2020 17:28
tomusdrw pushed a commit that referenced this pull request Mar 26, 2021
* use runtime:: prefix for message-lane pallet traces

* renamed message-lane (module and primitives) folder into messages

* replace "message lane" with "messages" where appropriate
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.