-
Notifications
You must be signed in to change notification settings - Fork 33
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Stargate with musselnet #69
Conversation
PO review testing workflows:
Other:
If you want to add cw20 allowance for me to test, my address is Notes for improvements
|
Listing claims has an error (I think this is when we moved from one item to a paginated list of claims): http://localhost:3000/stakes/claims/wasm1k0jntykt7e4g3y88ltc60czgjuqdy4c937v7dp gives |
What's the status here? It seems to me (reading commit history) that all of the open issues have been addressed. Please mark as "ready for review" when you feel it is ready for a final PR review. I will test it as a user, and ask @willclarktech to give it a quick pass for code. While this is all fresh in your head, can you make a new PR documenting all the steps you took here to make it work? Please include things like update ledger dependencies for new chrome, etc. |
packages/cw20-wallet/src/config.ts
Outdated
@@ -14,25 +14,26 @@ const local: AppConfig = { | |||
uatom: { denom: "ATOM", fractionalDigits: 6 }, | |||
}, | |||
gasPrice: 0.025, | |||
// TODO make sure it's correct | |||
codeId: 40, |
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.
What's this codeId for? The CW20 upload? I believe it's 2.
for (const denom in config.coinMap) { | ||
const coin = await client.getBalance(address, denom); | ||
if (coin) balance.push(coin); | ||
} |
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.
This is a pretty strange way of doing this. Why does this mutate an existing array rather than just returning the new balances and assigning them outside the function?
packages/name-service/src/config.ts
Outdated
@@ -14,25 +14,26 @@ const local: AppConfig = { | |||
uatom: { denom: "ATOM", fractionalDigits: 6 }, | |||
}, | |||
gasPrice: 0.025, | |||
// TODO make sure it's correct | |||
codeId: 5, |
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.
Are you uploading this yourself? We don't deploy it in the init
script.
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.
It's just a placeholder, I'll indicate that it needs to be filled with the code used when uploading
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.
For local apps, it is unclear.
I think it is 6 on musselnet (I added it with wasmd commands)
Maybe address this in another PR - proper local setup.
That should start up a blockchain, a faucet, and load all the contracts you want under predictable IDs (and a few instances of each). This is a fair bit of work (including getting proper setup for a staking contract to a real validator)
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 lot of codes deployed with predictable IDs via the scripts in CosmJS, but we don't currently deploy a name service there.
const typeRegistry = new Registry(); | ||
typeRegistry.register(msgStoreCodeTypeUrl, MsgStoreCode); | ||
typeRegistry.register(msgInstantiateContractTypeUrl, MsgInstantiateContract); | ||
typeRegistry.register(msgExecuteContractTypeUrl, MsgExecuteContract); |
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.
You shouldn't need to use a custom type registry with the latest release.
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.
Very nice. All x/wasm, x/bank, x/staking types are included by default?
So only if you have custom app-specific protos?
I guess this can lead to simplifying the migration guide
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.
Yes exactly, x/bank and x/staking are included in the SigningStargateClient
and x/wasm is added in SigningCosmWasmClient
.
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.
I did a second PO review and everything is working now.
Tested with both ledger and keplr in chrome. Great stuff.
I will create some issues for the outstanding comments from Will
Closes #67
Closes #56