wighawag / hardhat-deploy

hardhat deployment plugin
MIT License
1.18k stars 286 forks source link

Ledger path support #372

Closed stupid-boar closed 1 year ago

stupid-boar commented 1 year ago

master branch use only first account (path ignored)

Expected:

  namedAccounts: {
    deployer: {
      default: 0,
      5: "ledger://m/44'/60'/0'/0/0",
    },
  },
wighawag commented 1 year ago

this format ledger://m/44'/60'/0'/0/0 will not work, hardhat-deploy need to know the address before calling ledger

Maybe we could do something like ledger://m/44'/60'/0'/0/0:<address> ?

stupid-boar commented 1 year ago

@wighawag I made it work) I change the address to 0x0000...0 on config parsing

if (protocolSplit[0].toLowerCase() === 'ledger') {
    address = '0x0000000000000000000000000000000000000000';
    addressesToProtocol[address.toLowerCase()] = spec;

Then (transport init) I take PATH from protocol string (this is true, because hardware level)

ethersSigner = ledgerSignerFactory(
  provider,
  registeredProtocol.substr(9)
);

We have successfully deployed smart contract

wighawag commented 1 year ago

The issue is that namedAccounts are also used to references address, if you set it to zero address for all ledger then you cannot use that functionality

stupid-boar commented 1 year ago

I'll fix it soon ledger://m/44'/60'/0'/0/0:<address>

stupid-boar commented 1 year ago

@wighawag fixed)

wighawag commented 1 year ago

Nice! should have mentioned before, but we need to provide backward compatibility for those we put ledger://<address> in this case, the default path should be taken like before

stupid-boar commented 1 year ago

I'll fix it soon

stupid-boar commented 1 year ago

@wighawag done)

wei3erHase commented 1 year ago

🙏 !

stupid-boar commented 1 year ago

@wighawag it's done)

stupid-boar commented 1 year ago

@wighawag i fixed concurrent access to ledger device by using singleton pattern. Now it's work!)