Skip to content
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

Feature #1008 - Support new TXs for account module in Wallet #1033

Merged

Conversation

albertandrejev
Copy link
Contributor

Closes #1008.

@albertandrejev albertandrejev self-assigned this Feb 20, 2020
@albertandrejev albertandrejev changed the base branch from master to milestone/account-module-support March 4, 2020 16:41
@albertandrejev albertandrejev added this to the Account module support milestone Mar 4, 2020
@abefernan abefernan force-pushed the feature/#1008-account-txs-support branch from 75141eb to bd86d88 Compare March 6, 2020 09:52
@abefernan abefernan assigned abefernan and unassigned albertandrejev Mar 6, 2020
@abefernan abefernan removed this from the Account module support milestone Mar 6, 2020
@abefernan abefernan removed the WIP label Mar 6, 2020
@abefernan abefernan force-pushed the feature/#1008-account-txs-support branch from bd86d88 to 8f8201c Compare March 9, 2020 07:45
@albertandrejev albertandrejev force-pushed the milestone/account-module-support branch from e366bbf to 10c70c4 Compare March 9, 2020 11:04
Copy link
Contributor Author

@albertandrejev albertandrejev left a comment

Choose a reason for hiding this comment

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

Great job! Also as I am owner of this PR I can not approve it :)

{iovnames
.slice()
.sort((a, b) => {
if (a.username < b.username) return -1;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Better if you will use local compare function.

...
iovnames.slice().sort((a, b) =>
    a.username.localeCompare(b.username, undefined, { sensitivity: "base" }),
  );
...

Comment on lines 63 to 69
{starnames
.slice()
.sort((a, b) => {
if (`${a.name}*${a.domain}` < `${b.name}*${b.domain}`) return -1;
if (`${a.name}*${a.domain}` > `${b.name}*${b.domain}`) return 1;
return 0;
})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same here

@@ -85,7 +84,7 @@ withChainsDescribe("E2E > Receive Payment route", () => {
}, 35000);
});

describe("Starnames tab", () => {
describe.skip("Starnames tab", () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this ok, or you forgot to remove?

Copy link
Contributor

Choose a reason for hiding this comment

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

Since they need some rewrite I prefer to do it as part of #1039.

Comment on lines 7 to 9
export const REGISTER_IOVNAME_ROUTE = "/register-iovname";
export const REGISTER_STARNAME_ROUTE = "/register-starname";
export const REGISTER_NAME_ROUTE = "/register-name";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking about URL naming convention. What do you think if we will place it in order "noun"-"verb". In this case it should be /starname-register. Or even better /starname/register.

Copy link
Contributor

Choose a reason for hiding this comment

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

Great, I like the last option since it will open up other operations like /starname/renew or whatever.

@abefernan abefernan force-pushed the feature/#1008-account-txs-support branch from 6f45d89 to caa21cf Compare March 9, 2020 16:12
Copy link
Contributor Author

@albertandrejev albertandrejev left a comment

Choose a reason for hiding this comment

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

Great. I can not confirm, but sure you can by yourself.

@abefernan abefernan merged commit d700865 into milestone/account-module-support Mar 11, 2020
@abefernan abefernan deleted the feature/#1008-account-txs-support branch March 11, 2020 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support new transactions for starnames in Wallet
2 participants