Skip to content
This repository was archived by the owner on Jan 22, 2025. It is now read-only.

Add address map program#19473

Closed
jstarry wants to merge 1 commit intosolana-labs:masterfrom
jstarry:address-map-program
Closed

Add address map program#19473
jstarry wants to merge 1 commit intosolana-labs:masterfrom
jstarry:address-map-program

Conversation

@jstarry
Copy link
Copy Markdown
Contributor

@jstarry jstarry commented Aug 27, 2021

Problem

Users and protocols need a way to store addresses on-chain for transaction v2 address lookups.

Summary of Changes

Added address map program

  • Number of entries is capped at u8::MAX since transaction v2 account indices are typed as u8
  • Activation is instant but deactivation of on-chain map accounts has a cool down period to ensure that address maps can't be re-initialized at the same address.
  • Support closing address map accounts but use derived addresses based on the current epoch to prevent address maps from being re-initialized at the same address.

Fixes #

@jstarry jstarry force-pushed the address-map-program branch 7 times, most recently from 09db86c to bf31023 Compare September 1, 2021 00:47
@jstarry jstarry marked this pull request as ready for review September 1, 2021 00:48
@jstarry jstarry force-pushed the address-map-program branch 2 times, most recently from 0c51bea to addc847 Compare September 1, 2021 07:00
@jstarry jstarry force-pushed the address-map-program branch from addc847 to 8f4312d Compare September 6, 2021 18:54
@codecov
Copy link
Copy Markdown

codecov Bot commented Sep 6, 2021

Codecov Report

Merging #19473 (8f4312d) into master (8c5401e) will increase coverage by 12.1%.
The diff coverage is 28.5%.

❗ Current head 8f4312d differs from pull request most recent head d1f6225. Consider uploading reports for the commit d1f6225 to get more accurate results

@@             Coverage Diff             @@
##           master   #19473       +/-   ##
===========================================
+ Coverage    70.2%    82.3%    +12.1%     
===========================================
  Files          35      475      +440     
  Lines        2066   132434   +130368     
  Branches      295        0      -295     
===========================================
+ Hits         1451   109119   +107668     
- Misses        515    23315    +22800     
+ Partials      100        0      -100     

@jstarry jstarry requested a review from joncinque September 8, 2021 21:06
@jackcmay
Copy link
Copy Markdown
Contributor

jackcmay commented Sep 9, 2021

Should these be exposed via the sdk?

@jstarry
Copy link
Copy Markdown
Contributor Author

jstarry commented Sep 9, 2021

Should these be exposed via the sdk?

Yeah, definitely. I think I can use cargo features to have the sdk directly depend on the address map program crate without bringing in the program runtime.

Copy link
Copy Markdown
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

Looks really good! Most of my questions are nits and ideas about the interface, but I did look at the implementation as well, which looks safe

Comment thread programs/address-map/src/lib.rs
Comment thread programs/address-map/src/instruction.rs
Comment thread programs/address-map/src/processor.rs
Comment thread programs/address-map/src/processor.rs
Comment thread programs/address-map/src/processor.rs
}

/// Derive an address map address from a wallet address and the current epoch.
pub fn derive_address_map_address(payer_address: &Pubkey, current_epoch: Epoch) -> (Pubkey, u8) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm worried that this will make discoverability of address maps difficult. One of the great features of the associated token account is that you can immediately find someone's token account with just their wallet and the mint. Here, you may have to iterate through a bunch of epochs in order to find their address map, or you'll just use find_program_address and do it the "easy" way.

What if, instead of an epoch, this is another Pubkey? That way, you can setup a different address map for each program that you interact with. Since you've cleverly put this on a PDA dependent on the signer, there's no way for one of these to get hijacked after CloseAccount.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I agree that it's not ideal and I did consider this point when reimplementing the program here: #21616. However, I think indexing or creating a known registry of on-chain lookup tables is going to have to be the way forward here. I think it's more likely that bots and protocols will manage their own lookup table accounts and users will never have to manage them.

/// Returns an instruction that updates the authority of an address map account.
/// If the new authority is `None`, the address map account will be immutable.
/// Inactive address maps cannot be made immutable.
pub fn set_authority(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Similar to an associated token account, should we discourage people from reassigning authority? Immutability makes sense, since you may want to setup an address map for your program as a one-time procedure, and have it hard-coded into the program.

What are the use-cases for reassigning authority? I'm thinking that a user may create an address map for a program, then give the authority over to one of the program addresses which performs the setup and adjusts it as needed after updates.

&AddressMapInstruction::InitializeAccount {
bump_seed,
num_entries,
authority: authority_address,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

take it or leave it: Since we've had potential hijackings in the past, we may want to have a signature from the authority as well. This way, bad integrations that just look for any account map for an authority can't get duped.

For example, if it's a defi application, the attacker sets up trap address map accounts for a bunch of addresses, using their own token accounts as the receivers, and just hopes that a bad integration uses the trap address map.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Great call, I modified the create instruction here (#21616) to use the authority in the PDA and enforce that it's a signer. It's also no longer possible to set the authority address to another address.

Comment thread programs/address-map-tests/tests/processor.rs
Comment thread programs/address-map-tests/tests/processor.rs
@stale
Copy link
Copy Markdown

stale Bot commented Oct 2, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale Bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Oct 2, 2021
@jstarry jstarry force-pushed the address-map-program branch from 8f4312d to d1f6225 Compare November 26, 2021 03:37
@stale stale Bot removed the stale [bot only] Added to stale content; results in auto-close after a week. label Nov 26, 2021
@jstarry
Copy link
Copy Markdown
Contributor Author

jstarry commented Dec 5, 2021

Addressed all relevant comments and closing this implementation in favor of a more dev-friendly version here: #21616

@jstarry jstarry closed this Dec 5, 2021
@jstarry jstarry deleted the address-map-program branch January 5, 2022 03:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants