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

Contract Self-Identification NEP #129

Closed
wants to merge 7 commits into from

Conversation

luciotato
Copy link

@luciotato luciotato commented Oct 31, 2020

@luciotato luciotato changed the title Initial draft Contract Self-Identification (NEP-129) Oct 31, 2020
@luciotato luciotato changed the title Contract Self-Identification (NEP-129) Contract Self-Identification NEP Oct 31, 2020
Copy link
Collaborator

@bowenwang1996 bowenwang1996 left a comment

Choose a reason for hiding this comment

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

I feel like the part about auditing can potentially be its own NEP as there are a lot of potential problems there. Without a proper reputation system it seems that having the auditor's signature isn't really useful.

@evgenykuzyakov
Copy link
Contributor

Why does it have to be on-chain?
My current thinking is to be able to just link a metadata JSON file with a hash of it. It can either be stored on IPFS or github link with a commit hash, so it doesn't change. Then the metadata file can have the hash of the contract to link it back.

When you initialize a contract you pass a link to the metadata into the initialization argument and let's say the contract shouldn't change this link.

The metadata can contain all the required info. Audits can also go off-chain and link to a particular metadata file.

@luciotato
Copy link
Author

luciotato commented Nov 1, 2020

I'm open to all suggestions.
@evgenykuzyakov Do you think on-chain would be too expensive?
Please expand on "why not on chain", you've a ton of knowledge and it will be very useful if you can expand on that.

I'm putting all on-chain so you don't depend on nothing else than the chain, I'm with the idea that the more we use the chain for, the better (Maybe due to lack-of-knowledge I'm being too enthusiastic about it).

From Devs POV is better because you don't have to deal with other protocols/storage/concepts/documentation. If we use Github or IPFS, Devs need to read about/understand those services and also your code needs to include API's/libs to access those services. If everything is on-chain, you can get the data with the same near-cli/api you're already using.
If we add info/registry about auditors (as a DAO/community managed registry) then you can also check auditor reputation on-chain, you can even automatize that without leaving NEAR (e.g. "Get me a NEP-21 contract holding more than 1 million NEAR audited by someone with >98% reputation")

@luciotato
Copy link
Author

@bowenwang1996 Yes, the user must previously select auditors she trusts. But it's chicken-and-egg, this NEP could make the existence of a reputation system for auditors a necessity.

@evgenykuzyakov
Copy link
Contributor

My main reason for why not on-chain is that the most information doesn't have to be on-chain.

For example the contracts don't care about audits information, so it doesn't have to be on-chain so long as we can link this information to the contract. It can be linked if each audit has the account-id and the contract code hash for what's reviewed.

If we can securely link metadata file and the deployed contract, then we can put all required info into the metadata file.

If one contract wants to verify on-chain that the other contract satisfies the standard, by relying on an audit. This can be done if there is an auditor contract that whitelists contracts instead. Then you can rely on this whitelist to give you a verification that the given contract satisfies requirements. It's similarly how the Staking pool contracts are whitelisted for lockup contracts to delegate.

Eventually, all metadata can live on-chain and the link can just point to a contract on chain. But for now the audits have overhead and there are no need to validate them on-chain.

@luciotato
Copy link
Author

Heavy simplification: Based on reasonable feedback I've simplified the NEP limiting it to just contract self-identification and reducing on-chain required space to a minimum.

@vgrichina
Copy link

@luciotato should we also add URL with canonical web app for given contract? This can be useful for wallet a lot. /cc @kcole16

@luciotato
Copy link
Author

Good idea @vgrichina. Done.

@robert-zaremba
Copy link
Contributor

I started a discussion with an alternative approach: interface registry: #154

@frol frol added the WG-contract-standards Contract Standards Work Group should be accountable label Sep 5, 2022
Copy link
Member

@mfornet mfornet left a comment

Choose a reason for hiding this comment

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

Left some comments with regard to the format of the document (syntax only)

@@ -0,0 +1,207 @@
# Contract Self-Identification ([NEP-129](https://github.com/nearprotocol/NEPs/pull/129))
Copy link
Member

Choose a reason for hiding this comment

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

Add proper header per NEP-1. See template.

Copy link
Member

Choose a reason for hiding this comment

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

Add Rationale and Alternative version

Copy link
Member

Choose a reason for hiding this comment

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

Add Copyright?

Prior art:
- TBD

## Guide-level explanation
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
## Guide-level explanation
## Specification

Comment on lines +78 to +81
## Reference-level explanation
[reference-level-explanation]: #reference-level-explanation

### Reference Implementations:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
## Reference-level explanation
[reference-level-explanation]: #reference-level-explanation
### Reference Implementations:
## Reference Implementations

}
```

### Details on the fields:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
### Details on the fields:
### Details on the fields

Remove two points for headers

}
```

#### ts/pseudocode:
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion, consider moving, ts implementation to assets, and link it here (mostly if it is pseudocode).

Comment on lines +83 to +126
#### Rust:
```rust
// ---------------------------------
// -- CONTRACT Self Identification --
// ---------------------------------
// [NEP-129](https://github.com/nearprotocol/NEPs/pull/129)
// see also pub fn get_contract_info
pub const CONTRACT_NAME: &str = "diversifying staking pool";
pub const CONTRACT_VERSION: &str = "0.1.0";
pub const DEVELOPERS_ACCOUNT_ID: &str = "developers.near";
pub const DEFAULT_WEB_APP_URL: &str = "http://div-pool.narwallets.com";
pub const DEFAULT_AUDITOR_ACCOUNT_ID: &str = "auditors.near";

/// NEP-129 get information about this contract
/// returns JSON string according to [NEP-129](https://github.com/nearprotocol/NEPs/pull/129)
/// Rewards fee fraction structure for the staking pool contract.
#[derive(Serialize)]
#[serde(crate = "near_sdk::serde")]
#[allow(non_snake_case)]
pub struct NEP129Response {
pub dataVersion:u16,
pub name:String,
pub version:String,
pub developersAccountId:String,
pub source:String,
pub standards:Vec<String>,
pub webAppUrl:Option<String>,
pub auditorAccountId:Option<String>,
}

/// NEP-129 get information about this contract
/// returns JSON string according to [NEP-129](https://github.com/nearprotocol/NEPs/pull/129)
pub fn get_contract_info(&self) -> NEP129Response {
return NEP129Response {
dataVersion:1,
name: CONTRACT_NAME.into(),
version:CONTRACT_VERSION.into(),
developersAccountId:DEVELOPERS_ACCOUNT_ID.into(),
source:"https://github.com/Narwallets/diversifying-staking-pool".into(),
standards:vec!("NEP-129".into()),
webAppUrl:self.web_app_url.clone(),
auditorAccountId:self.auditor_account_id.clone()
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I found the current code distracting. I think either pseudo code is better (potentially moving this implementation to assets) and making sure it compiles (if copy pasted properly in the project).

Otherwise, let's make sure the code is properly formatted, get rid of global variables given they are used only once, properly update comments so they are more useful.

version:CONTRACT_VERSION.into(),
developersAccountId:DEVELOPERS_ACCOUNT_ID.into(),
source:"https://github.com/Narwallets/diversifying-staking-pool".into(),
standards:vec!("NEP-129".into()),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
standards:vec!("NEP-129".into()),
standards:vec!["NEP-129".into()],

Comment on lines +10 to +19
## Changelog

### `0.1.0`

- Heavy simplification. Only contract identification.
- added webAppUrl per @vgrichina suggestion

### `0.0.1`

- Initial Proposal
Copy link
Member

Choose a reason for hiding this comment

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

Remove. Add vgrichina as coauthor if necessary.

@mfornet
Copy link
Member

mfornet commented Sep 18, 2022

There are two alternative to this NEP: #154 and #275. Both of them are very similar, they propose to create an on-chain registry with the information of which NEP is supported by each contract.

Advantage of registry vs builtin self-identification:

  • Add NEP discoverability for inmutable contract in hindsight.

Con:

  • A trustless mechanism should be created to add and fetch elements to the registry. See approach suggested in Registry Standard #275 (using endorsements)

There is almost no difference about having the information in a registry vs inside the contract itself for discoverability. Off-chain tools can be built either way. For on-chain contracts that want to discover if a particular standard is implemented, they will need to make a xcc, and react on that information on the callback. It is not particularly relevant if the queried contract is the target contract or a registry.

@frol frol added help wanted Extra attention is needed S-draft/needs-author-revision A NEP in the DRAFT stage that needs an author revision. labels Sep 30, 2022
@ori-near ori-near added the A-NEP A NEAR Enhancement Proposal (NEP). label Oct 13, 2022
@frol frol mentioned this pull request Dec 12, 2022
2 tasks
@ori-near ori-near marked this pull request as draft March 20, 2023 17:55
@frol
Copy link
Collaborator

frol commented May 23, 2023

I am closing this NEP as it seems that it was superseded with NEP #330 which was later extended with #351. There are still some fields missing that were proposed in this NEP, so if anyone wants to extend NEP #330, feel free to create a NEP extension (PR with changes to the existing NEP document).

@frol frol closed this May 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-NEP A NEAR Enhancement Proposal (NEP). help wanted Extra attention is needed S-draft/needs-author-revision A NEP in the DRAFT stage that needs an author revision. WG-contract-standards Contract Standards Work Group should be accountable
Projects
Status: REJECTED
Development

Successfully merging this pull request may close these issues.

10 participants