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

CPS-???? | Coin Selection Including Native Tokens #609

Closed
wants to merge 5 commits into from

Conversation

HinsonSIDAN
Copy link
Contributor

@HinsonSIDAN HinsonSIDAN commented Oct 20, 2023

This seems a long withstanding issue (#232) where most offline library we got now still using sub-optimal coin selection strategy which might lead to inefficiency transaction building process.

Raising this CPS in an attempt to attract community's attraction to solve this issue with combined intelligence.

@rphair rphair changed the title CPS 2? - Coin Selection Approach for Post Native Tokens Era in Cardano CPS-???? - Coin Selection Including Native Tokens Oct 21, 2023
Copy link
Collaborator

@rphair rphair left a comment

Choose a reason for hiding this comment

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

I've edited the title down for specificity and to remove redundancy.

Note there was a proposed CPS-0002 which in my opinion still represents an unresolved issue which could be reactivated: #374

In any case, when skipping over numbers due to deprecations or abandonments we don't go back and fill in the missing numbers, but would rather choose the next highest number in sequence after the last assignment.

In general I think it's good to have this as a CPS so thanks @SIDANWhatever for your submission & looking forward to discussing this in the community soon.

@@ -0,0 +1,93 @@
---
CPS: 2
Copy link
Collaborator

@rphair rphair Oct 21, 2023

Choose a reason for hiding this comment

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

Suggested change
CPS: 2
CPS: ?

Please @SIDANWhatever you will also need to rename the containing directory from CPS-0002 to something non-numerical (like CPS-XXXX or CPS-token-selection.

@@ -0,0 +1,93 @@
---
CPS: 2
Title: Coin Selection Approach for Post Native Tokens Era in Cardano
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Title: Coin Selection Approach for Post Native Tokens Era in Cardano
Title: Coin Selection Including Native Tokens

If author & editors are in agreement that this is more specific and less redundant ("era", "Cardano") then let's please change the document title to the current PR title.

Copy link
Collaborator

Choose a reason for hiding this comment

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

p.s. also @SIDANWhatever for fixing little things like this we recommend that your PR branch be set to "Allow edits by maintainers" (the general default when submitting a PR, and helpful to keep track of who recommended the changes behind each commit).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

p.s. also @SIDANWhatever for fixing little things like this we recommend that your PR branch be set to "Allow edits by maintainers" (the general default when submitting a PR, and helpful to keep track of who recommended the changes behind each commit).

Seems its only work for PR from feature branch, should I close this PR and create a new one?

Copy link
Collaborator

@rphair rphair Oct 21, 2023

Choose a reason for hiding this comment

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

no need to create a new PR... you should be able to extend the permissions by following these instructions (i.e. when you view this PR's branch in the repository which pushed it, with write permission in that repository) 🤓 https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork#enabling-repository-maintainer-permissions-on-existing-pull-requests

@rphair rphair added the Category: Wallets Proposals belonging to the 'Wallets' category. label Oct 21, 2023
@rphair rphair changed the title CPS-???? - Coin Selection Including Native Tokens CPS-???? | Coin Selection Including Native Tokens Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Wallets Proposals belonging to the 'Wallets' category.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants